laskoviymishka commented on code in PR #3473:
URL: https://github.com/apache/iceberg-rust/pull/3473#discussion_r4323246355
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -193,10 +193,10 @@ impl FileScanTaskReader {
let need_row_number = project_pos || (project_row_id &&
task.first_row_id().is_some());
let field_ids = task.project_field_ids();
- let metadata_only_projection = !field_ids.is_empty()
- && field_ids
- .iter()
- .all(|id| PRUNABLE_METADATA_FIELDS.contains(id));
+ // An empty projection (e.g. `COUNT(*)`) is vacuously metadata-only.
+ let metadata_only_projection = field_ids
Review Comment:
Follow-up: `metadata_only_projection` now also covers "nothing projected",
and the name doesn't say that. I'd rename it to something like
`no_data_columns_projected`, and add "(including the empty projection)" to the
`PRUNABLE_METADATA_FIELDS` doc at L111. The vacuous `.all()` against that
constant is now what makes `COUNT(*)` prune, so a non-empty guard could easily
come back without anyone noticing.
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -3376,6 +3374,32 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_empty_projection_reads_no_data_columns() {
+ let tmp_dir = TempDir::new().unwrap();
+ let dir = tmp_dir.path().to_str().unwrap();
+
+ let empty = metadata_projection_task(
+ write_parquet_with_wide_column(dir, "empty.parquet", vec![],
vec![]),
+ id_and_wide_schema(),
+ vec![],
+ );
+ let (_, empty_bytes) = scan_task(empty).await;
Review Comment:
The batches are dropped here, so the test shows fewer bytes were read but
not that the row count survived the `none` mask. Small follow-up: check the
output shape in the same test, the way the `_pos` / `_file` sibling tests do,
e.g.
```rust
let (batches, empty_bytes) = scan_task(empty).await;
assert!(batches.iter().all(|b| b.num_columns() == 0));
assert_eq!(batches.iter().map(|b| b.num_rows()).sum::<usize>(),
expected_rows);
```
with `expected_rows` being the fixture's row count.
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -297,13 +297,11 @@ impl FileScanTaskReader {
use_position_fallback, // Whether to use position-based (true) or
field-ID-based (false) projection
)?;
- // A metadata-only projection leaves
`project_field_ids_without_metadata` empty,
- // which `get_arrow_projection_mask` maps to "read all columns" (so
`COUNT(*)` still
- // gets a row count). Downgrade that to "read no data columns":
`install_row_number`
- // put the RowNumber virtual column on every metadata-only projection
as a row-count
- // source independent of the data columns, so the count survives with
zero data
- // columns read. `COUNT(*)` (an empty projection) has no RowNumber and
keeps reading
- // all columns to preserve the row count.
+ // A metadata-only or empty projection leaves
`project_field_ids_without_metadata`
+ // empty, which `get_arrow_projection_mask` maps to "read all
columns". Downgrade that
+ // to "read no data columns": `install_row_number` put the RowNumber
virtual column on
+ // every such projection as a row-count source independent of the data
columns, so the
+ // count survives with zero data columns read.
Review Comment:
This is now the `COUNT(*)` path for every table, deletes included, so the
count has to survive `ProjectionMask::none` together with a `RowSelection`
(positional deletes) and a `RowFilter` (equality deletes / predicate). I think
it does, since the predicate carries its own mask, but nothing tests it with
zero output columns.
Fine as a follow-up: next to `test_empty_projection_preserves_row_count`, an
empty projection with one positional delete asserting N-1 rows, and an empty
projection with a predicate on a non-projected column asserting the filtered
count.
--
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]
