kevinjqliu commented on code in PR #3631:
URL: https://github.com/apache/iceberg-rust/pull/3631#discussion_r4495862490
##########
crates/iceberg/src/arrow/manifest.rs:
##########
@@ -142,30 +134,128 @@ pub fn read_manifest_entries(bytes: &[u8],
manifest_file: &ManifestFile) -> Resu
.map_err(invalid_manifest)?;
}
- let Some(batch) = reader.next() else {
+ let Some(batch) = only_batch(reader, invalid_manifest)? else {
Review Comment:
we might want to revisit this once the data reach a certain size
##########
crates/iceberg/src/spec/manifest/entry.rs:
##########
@@ -177,6 +177,17 @@ impl TryFrom<i32> for ManifestStatus {
use super::DataFileFormat;
+/// Field ID of `manifest_entry.data_file`.
+pub(crate) const DATA_FILE_FIELD_ID: i32 = 2;
+/// Field ID of `manifest_entry.sequence_number`.
+pub(crate) const SEQUENCE_NUMBER_FIELD_ID: i32 = 3;
+/// Field ID of `manifest_entry.file_sequence_number`.
+pub(crate) const FILE_SEQUENCE_NUMBER_FIELD_ID: i32 = 4;
+/// Field ID of `data_file.partition`.
+pub(crate) const DATA_FILE_PARTITION_FIELD_ID: i32 = 102;
+/// Field ID of `data_file.content`.
+pub(crate) const DATA_FILE_CONTENT_FIELD_ID: i32 = 134;
Review Comment:
nice, ty for the refactor
##########
crates/iceberg/src/spec/manifest/entry.rs:
##########
@@ -177,6 +177,17 @@ impl TryFrom<i32> for ManifestStatus {
use super::DataFileFormat;
+/// Field ID of `manifest_entry.data_file`.
+pub(crate) const DATA_FILE_FIELD_ID: i32 = 2;
+/// Field ID of `manifest_entry.sequence_number`.
+pub(crate) const SEQUENCE_NUMBER_FIELD_ID: i32 = 3;
+/// Field ID of `manifest_entry.file_sequence_number`.
+pub(crate) const FILE_SEQUENCE_NUMBER_FIELD_ID: i32 = 4;
+/// Field ID of `data_file.partition`.
+pub(crate) const DATA_FILE_PARTITION_FIELD_ID: i32 = 102;
+/// Field ID of `data_file.content`.
+pub(crate) const DATA_FILE_CONTENT_FIELD_ID: i32 = 134;
Review Comment:
i see `manifest_list` has a `_const_schema.rs` file, maybe we should do the
same. thats a bigger refactor, lets not do it here
##########
crates/iceberg/src/arrow/manifest.rs:
##########
@@ -142,30 +134,128 @@ pub fn read_manifest_entries(bytes: &[u8],
manifest_file: &ManifestFile) -> Resu
.map_err(invalid_manifest)?;
}
- let Some(batch) = reader.next() else {
+ let Some(batch) = only_batch(reader, invalid_manifest)? else {
return Ok(RecordBatch::new_empty(schema));
};
- let batch = batch.map_err(invalid_manifest)?;
- // Taking only the first batch relies on arrow-avro filling it to the batch
- // size, so fail instead of dropping entries if that changes.
- if reader.next().is_some() {
- return Err(invalid_data!(
- "arrow-avro split the manifest into more than one batch"
- ));
- }
+ // v1 manifests have no content or sequence number columns, which v2+
Review Comment:
meta question. what do you think is the best way to deal with different
manifest versions in terms of arrow schema?
for example, say we have a table that is upgraded from v1 all the way to v4.
how should we represent all the fields? should the arrow schema be a superset
of all the versions combined?
and then how do we validate the integrity of the data? for example, "field x
should not exist in v1 schema"?
just curious, i feel like we have to answer this eventually 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
