comphead commented on code in PR #3473:
URL: https://github.com/apache/iceberg-rust/pull/3473#discussion_r4328291916
##########
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:
Done in b97860abc. Renamed to `no_data_columns_projected`, added "(including
the empty projection, e.g. `COUNT(*)`)" to the `PRUNABLE_METADATA_FIELDS`
comment, and noted at the `.all()` that it is vacuously true for an empty
projection so there should be no `is_empty()` guard.
##########
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:
Added both in b97860abc:
`test_empty_projection_counts_rows_after_positional_delete` (one delete,
expects 2 of 3 rows) and `test_empty_projection_counts_rows_after_predicate`
(`id != 2` on an unprojected column, expects 2 rows). Mutation check: skipping
the delete selection fails the first, and dropping the row filter fails the
second. Giving the row filter an empty mask does not fail either test, so they
pin the observable counts but not that internal detail.
##########
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:
Done in b97860abc. `test_empty_projection_reads_no_data_columns` now keeps
the batches and asserts every batch has zero columns and the total row count is
3.
--
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]
