You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "mbutrovich (via GitHub)" <gi...@apache.org> on 2026/06/10 18:04:11 UTC
[PR] fix(reader): `filter_row_groups_by_byte_range` duplicates rows for sub-row-group file splits [iceberg-rust]
mbutrovich opened a new pull request, #2615:
URL: https://github.com/apache/iceberg-rust/pull/2615
## Which issue does this PR close?
- Closes #2614.
## What changes are included in this PR?
`ArrowReader::filter_row_groups_by_byte_range` (added in #1779) selected a row group for every `FileScanTask` byte range that *overlapped* it. When a data file is split into byte ranges smaller than a row group, multiple tasks overlap the same row group, so each reads it and the rows are duplicated. This surfaced in Apache DataFusion Comet ([apache/datafusion-comet#4590](https://github.com/apache/datafusion-comet/issues/4590)), where Spark tiles a file by `split-size` regardless of row-group layout.
This PR switches selection to *midpoint ownership*, matching parquet-java's [`ParquetMetadataConverter.filterFileMetaDataByMidpoint`](https://github.com/apache/parquet-java/blob/master/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java#L1287-L1290): a split owns a row group only if its `[start, start+length)` range contains the row group's midpoint. Because the splits tile the file contiguously and disjointly, exactly one split contains a given midpoint, so each row group is read once. The comparison is half-open (`start <= midpoint < end`) so a midpoint landing on a split boundary belongs to the upper split.
This is a no-op for whole-file tasks (`start=0, length=file_size`, all iceberg-rust's own planner emits), since every midpoint lies in range and all row groups are selected. It only changes the externally-planned sub-row-group split case, completing the byte-range work from #1779.
## Are these changes tested?
Yes. A new test `test_sub_row_group_splits_do_not_duplicate_rows` writes a 3-row-group file, tiles it into 64-byte splits, reads every split through `ArrowReader`, and asserts each row appears exactly once. It returns ~2800 rows before the fix and exactly 300 after. The existing `test_file_splits_respect_byte_ranges` (boundary-aligned splits) continues to pass.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): `filter_row_groups_by_byte_range` duplicates rows for sub-row-group file splits [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #2615:
URL: https://github.com/apache/iceberg-rust/pull/2615#issuecomment-4679523503
Thanks for the quick reviews @advancedxy and @blackmwk! Hopefully I addressed all feedback in https://github.com/apache/iceberg-rust/pull/2615/changes/46dde2cc9eb128f66940878d183e06cb60ffc283. All new tests fail without the fix in place.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): `filter_row_groups_by_byte_range` duplicates rows for sub-row-group file splits [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #2615:
URL: https://github.com/apache/iceberg-rust/pull/2615#issuecomment-4680927408
Thanks again to everyone for the quick response on this correctness issue. Comet will bump to this commit on `main` today.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): `filter_row_groups_by_byte_range` duplicates rows for sub-row-group file splits [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2615:
URL: https://github.com/apache/iceberg-rust/pull/2615#discussion_r3395002730
##########
crates/iceberg/src/arrow/reader/row_filter.rs:
##########
@@ -160,8 +160,14 @@ impl ArrowReader {
/// Filters row groups by byte range to support Iceberg's file splitting.
///
- /// Iceberg splits large files at row group boundaries, so we only read row groups
- /// whose byte ranges overlap with [start, start+length).
+ /// External engines (e.g. Spark via Comet) split a data file into multiple scan tasks,
Review Comment:
That's good context, thank you!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): `filter_row_groups_by_byte_range` duplicates rows for sub-row-group file splits [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2615:
URL: https://github.com/apache/iceberg-rust/pull/2615#discussion_r3395003926
##########
crates/iceberg/src/arrow/reader/row_filter.rs:
##########
@@ -604,4 +612,102 @@ mod tests {
assert_eq!(first_val, 100, "Task 2 should start with id=100, not id=0");
}
}
+
+ /// A single data file split into multiple sub-row-group byte ranges (as Spark/Iceberg
+ /// planning produces when split-size is smaller than a row group) must still yield each
+ /// row exactly once. The previous overlap-based selection let every split whose byte range
+ /// touched a row group read it, duplicating rows; ownership by midpoint reads each row group
+ /// from exactly one split.
+ #[tokio::test]
+ async fn test_sub_row_group_splits_do_not_duplicate_rows() {
+ use arrow_array::Int32Array;
+
+ let schema = Arc::new(
+ Schema::builder()
+ .with_schema_id(1)
+ .with_fields(vec![
+ NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(),
+ ])
+ .build()
+ .unwrap(),
+ );
+
+ let arrow_schema = Arc::new(ArrowSchema::new(vec![
+ Field::new("id", DataType::Int32, false).with_metadata(HashMap::from([(
+ PARQUET_FIELD_ID_META_KEY.to_string(),
+ "1".to_string(),
+ )])),
+ ]));
+
+ let tmp_dir = TempDir::new().unwrap();
+ let table_location = tmp_dir.path().to_str().unwrap().to_string();
+ let file_path = format!("{table_location}/sub_split.parquet");
+
+ // Three row groups of 100 rows each (ids 0..300).
+ let props = WriterProperties::builder()
+ .set_compression(Compression::SNAPPY)
+ .set_max_row_group_row_count(Some(100))
+ .build();
+ let file = File::create(&file_path).unwrap();
+ let mut writer = ArrowWriter::try_new(file, arrow_schema.clone(), Some(props)).unwrap();
+ for chunk in [0..100, 100..200, 200..300] {
+ let batch = RecordBatch::try_new(arrow_schema.clone(), vec![Arc::new(
+ Int32Array::from(chunk.collect::<Vec<i32>>()),
+ )])
+ .unwrap();
+ writer.write(&batch).expect("Writing batch");
+ }
+ writer.close().unwrap();
+
+ let file_size = std::fs::metadata(&file_path).unwrap().len();
+ let file_io = FileIO::new_with_fs();
+ let reader = ArrowReaderBuilder::new(file_io, Runtime::current()).build();
+
+ // Tile the whole file into 64-byte splits, mirroring Spark's split-size planning, and
+ // read every split. A 64-byte split is far smaller than a row group, so each row group
+ // is touched by several splits but must be owned (read) by exactly one.
+ let mut ids = Vec::new();
+ let split_size = 64u64;
+ let mut start = 0u64;
+ while start < file_size {
+ let length = split_size.min(file_size - start);
+ let task = FileScanTask::builder()
+ .with_file_size_in_bytes(file_size)
+ .with_start(start)
Review Comment:
Will do, good suggestion!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): `filter_row_groups_by_byte_range` duplicates rows for sub-row-group file splits [iceberg-rust]
Posted by "jordepic (via GitHub)" <gi...@apache.org>.
jordepic commented on PR #2615:
URL: https://github.com/apache/iceberg-rust/pull/2615#issuecomment-4679951054
Thanks Matt! I just encountered this issue yesterday from comet. Using the midpoint for row group -> split assignment is what is done in the parquet-java library.
For context, the FileScanTask API allows providing custom split ranges - however this doesn't really make sense in scenarios with huge parquet row groups where many of the ranges intercept one row group. We'll read back duplicate data, which is bad!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): `filter_row_groups_by_byte_range` duplicates rows for sub-row-group file splits [iceberg-rust]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #2615:
URL: https://github.com/apache/iceberg-rust/pull/2615#discussion_r3393816511
##########
crates/iceberg/src/arrow/reader/row_filter.rs:
##########
@@ -160,8 +160,14 @@ impl ArrowReader {
/// Filters row groups by byte range to support Iceberg's file splitting.
///
- /// Iceberg splits large files at row group boundaries, so we only read row groups
- /// whose byte ranges overlap with [start, start+length).
+ /// External engines (e.g. Spark via Comet) split a data file into multiple scan tasks,
Review Comment:
I think the comment could be updated to reflect the fact: at most(normal) cases the iceberg parquet files are split at row group boundaries. It only split parquet files at request size if the splitOffsets metadata is missing when planning.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): `filter_row_groups_by_byte_range` duplicates rows for sub-row-group file splits [iceberg-rust]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #2615:
URL: https://github.com/apache/iceberg-rust/pull/2615#discussion_r3393866757
##########
crates/iceberg/src/arrow/reader/row_filter.rs:
##########
@@ -604,4 +612,102 @@ mod tests {
assert_eq!(first_val, 100, "Task 2 should start with id=100, not id=0");
}
}
+
+ /// A single data file split into multiple sub-row-group byte ranges (as Spark/Iceberg
+ /// planning produces when split-size is smaller than a row group) must still yield each
+ /// row exactly once. The previous overlap-based selection let every split whose byte range
+ /// touched a row group read it, duplicating rows; ownership by midpoint reads each row group
+ /// from exactly one split.
+ #[tokio::test]
+ async fn test_sub_row_group_splits_do_not_duplicate_rows() {
+ use arrow_array::Int32Array;
+
+ let schema = Arc::new(
+ Schema::builder()
+ .with_schema_id(1)
+ .with_fields(vec![
+ NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(),
+ ])
+ .build()
+ .unwrap(),
+ );
+
+ let arrow_schema = Arc::new(ArrowSchema::new(vec![
+ Field::new("id", DataType::Int32, false).with_metadata(HashMap::from([(
+ PARQUET_FIELD_ID_META_KEY.to_string(),
+ "1".to_string(),
+ )])),
+ ]));
+
+ let tmp_dir = TempDir::new().unwrap();
+ let table_location = tmp_dir.path().to_str().unwrap().to_string();
+ let file_path = format!("{table_location}/sub_split.parquet");
+
+ // Three row groups of 100 rows each (ids 0..300).
+ let props = WriterProperties::builder()
+ .set_compression(Compression::SNAPPY)
+ .set_max_row_group_row_count(Some(100))
+ .build();
+ let file = File::create(&file_path).unwrap();
+ let mut writer = ArrowWriter::try_new(file, arrow_schema.clone(), Some(props)).unwrap();
+ for chunk in [0..100, 100..200, 200..300] {
+ let batch = RecordBatch::try_new(arrow_schema.clone(), vec![Arc::new(
+ Int32Array::from(chunk.collect::<Vec<i32>>()),
+ )])
+ .unwrap();
+ writer.write(&batch).expect("Writing batch");
+ }
+ writer.close().unwrap();
+
+ let file_size = std::fs::metadata(&file_path).unwrap().len();
+ let file_io = FileIO::new_with_fs();
+ let reader = ArrowReaderBuilder::new(file_io, Runtime::current()).build();
+
+ // Tile the whole file into 64-byte splits, mirroring Spark's split-size planning, and
+ // read every split. A 64-byte split is far smaller than a row group, so each row group
+ // is touched by several splits but must be owned (read) by exactly one.
+ let mut ids = Vec::new();
+ let split_size = 64u64;
+ let mut start = 0u64;
+ while start < file_size {
+ let length = split_size.min(file_size - start);
+ let task = FileScanTask::builder()
+ .with_file_size_in_bytes(file_size)
+ .with_start(start)
Review Comment:
it might be helpful to test cases where the start of file scan task being exactly of the mid of one row group.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): `filter_row_groups_by_byte_range` duplicates rows for sub-row-group file splits [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk merged PR #2615:
URL: https://github.com/apache/iceberg-rust/pull/2615
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): `filter_row_groups_by_byte_range` duplicates rows for sub-row-group file splits [iceberg-rust]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #2615:
URL: https://github.com/apache/iceberg-rust/pull/2615#issuecomment-4678690127
> Hi, @mbutrovich I don't think this is the problem of row filter. Instead, this should be a bug of task planning algorithm.
Hi, @blackmwk I don't think the task planning algorithm can always produce the correct row group offset for parquet files. As I commented in the linked issue https://github.com/apache/datafusion-comet/issues/4590, if the datafile entry's split offset is missing which could be possible for bad writer behavior or it's created manually.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org