laskoviymishka commented on code in PR #3117:
URL: https://github.com/apache/iceberg-rust/pull/3117#discussion_r3892655644
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -230,7 +230,13 @@ impl FileScanTaskReader {
// column, so nothing is synthesized and the column is not needed.
let need_row_number = project_pos || (project_row_id &&
task.first_row_id.is_some());
- let arrow_metadata = if need_row_number {
+ let field_ids = task.project_field_ids();
+ let metadata_only_projection =
+ !field_ids.is_empty() && field_ids.iter().all(|&id|
is_metadata_field(id));
Review Comment:
This predicate is broader than the set of columns we actually materialize in
`process()`. `is_metadata_field` returns `true` for
`RESERVED_FIELD_ID_DELETED`, but there's no `with_constant` /
`with_virtual_field` / `with_null_metadata_column` branch for `_deleted` below
-- unlike the six we do handle (`_file`, `_spec_id`, `_partition`, `_pos`,
`_row_id`, `_last_updated_sequence_number`).
So a `_deleted`-only projection now flips `metadata_only_projection` true,
installs RowNumber, and downgrades to `ProjectionMask::none()`; the transformer
then gets N rows with only the RowNumber column and no handler for `_deleted`,
so the output batch has zero user-visible columns. It was probably already
emitting the wrong schema before this -- what changes is we now prune to
footer-only, so the gap gets masked behind an I/O win instead of surfaced.
I'd gate the pruning on the columns that actually have a handler rather than
the full `is_metadata_field` set:
```rust
const PRUNABLE_METADATA_FIELDS: &[i32] = &[
RESERVED_FIELD_ID_FILE,
RESERVED_FIELD_ID_SPEC_ID,
RESERVED_FIELD_ID_PARTITION,
RESERVED_FIELD_ID_POS,
RESERVED_FIELD_ID_ROW_ID,
RESERVED_FIELD_ID_LAST_UPDATED_SEQUENCE_NUMBER,
];
let metadata_only_projection =
!field_ids.is_empty() && field_ids.iter().all(|id|
PRUNABLE_METADATA_FIELDS.contains(id));
```
That keeps `_deleted` on the read-all path until it grows a real handler.
wdyt?
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -3300,8 +3454,8 @@ mod tests {
metadata_projection_task(file_path.clone(), schema,
vec![RESERVED_FIELD_ID_FILE]);
let (batches, _) = scan_task(task).await;
- // A pure-constant projection has no independent row source, so the
row count must
- // still come from the file (the `empty -> all()` path is preserved
for this case).
+ // A metadata-only constant projection is pruned to zero data columns;
RowNumber
+ // supplies the row count and the `_file` constant is sized from it (3
rows).
let total_rows: usize = batches.iter().map(|b| b.num_rows()).sum();
assert_eq!(total_rows, 3);
Review Comment:
This test's behavior flipped (all columns -> zero data columns) but it still
only asserts `total_rows == 3` and the `_file` value, so a regression that
re-enabled data reads -- or leaked the RowNumber column into the output --
would pass. The three new tests all assert `num_columns() == 1` and compare
`bytes_read`; I'd bring this one up to match.
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -3246,8 +3400,8 @@ mod tests {
// Seq-only projection with a null first_row_id: the column is nulled
and the
// physical leaf is NOT read (the gated
`coalesce_last_updated_seq_leaf` is None).
- // The downgrade must therefore not fire -- keying off the raw
`project_*` flag
- // instead would drop the only readable column and lose the row count.
+ // This is a metadata-only projection, so RowNumber is installed as
the row-count
+ // source and the data columns are pruned; the row count must still be
3.
Review Comment:
Same here -- this reversed from reading-all to pruned but kept just
`total_rows == 3` and the all-null values. The sibling
`test_row_id_only_null_first_row_id_reads_no_data_columns` added in this PR
compares `bytes_read`, so mirroring it keeps the two symmetric and actually
guards the pruning this now describes.
--
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]
