mbutrovich commented on code in PR #2746:
URL: https://github.com/apache/iceberg-rust/pull/2746#discussion_r3501020482
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -2369,4 +2369,68 @@ pub mod tests {
// Assert it finished (didn't timeout)
assert!(result.is_ok(), "Scan timed out - deadlock detected");
}
+
+ #[tokio::test]
+ async fn test_select_with_pos_column() {
+ use arrow_array::cast::AsArray;
+
+ let mut fixture = TableTestFixture::new();
+ fixture.setup_manifest_files().await;
+
+ // Select regular columns plus the _pos column
+ let table_scan = fixture
+ .table
+ .scan()
+ .select(["x", RESERVED_COL_NAME_POS])
+ .with_row_selection_enabled(true)
Review Comment:
With no filter, all 1024 rows are read contiguously, so this passes whether
or not `_pos` survives selection correctly. The interesting case, and the
reason to use the reader's row-number column rather than a counter, is a
predicate that prunes row groups/rows, where `_pos` must still report absolute
file positions (for example non-contiguous like `[0,1,2, 500,501,...]`). Could
we add that? It's the test that proves the design.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -2369,4 +2369,68 @@ pub mod tests {
// Assert it finished (didn't timeout)
assert!(result.is_ok(), "Scan timed out - deadlock detected");
}
+
+ #[tokio::test]
Review Comment:
The spec defines `_pos` as the ordinal position in the source data file
(https://iceberg.apache.org/spec/#reserved-field-ids). Two behaviors I'd love a
test for, next to the ones you added here: (a) applying a delete filter doesn't
renumber `_pos`; (b) a task scanning only `[start, start+length)` still emits
absolute positions, matching Java's `findStartingRowPos`. Both look reachable
with the existing `TableTestFixture`. Do these hold today?
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -207,6 +209,36 @@ impl FileScanTaskReader {
arrow_metadata
};
+ let project_pos =
task.project_field_ids().contains(&RESERVED_FIELD_ID_POS);
+
+ let arrow_metadata = if project_pos {
+ let row_number_field = Arc::new(
+ Field::new("row_number", DataType::Int64, false)
+ .with_metadata(HashMap::from([(
+ PARQUET_FIELD_ID_META_KEY.to_string(),
Review Comment:
`Error::new` takes `impl Into<String>`, and the sibling call near L179
passes a `&str` literal directly. Removing `.to_string()` matches that.
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -207,6 +209,36 @@ impl FileScanTaskReader {
arrow_metadata
};
+ let project_pos =
task.project_field_ids().contains(&RESERVED_FIELD_ID_POS);
+
+ let arrow_metadata = if project_pos {
Review Comment:
There are now up to three sequential `ArrowReaderMetadata::try_new` rebuilds
per file (field-ID schema, INT96 coercion, virtual columns), each re-deriving
from parquet metadata. Could the final `ArrowReaderOptions` (schema plus
virtual columns) be assembled once and `try_new` called a single time? Minor
and not blocking; it just compounds with the two existing passes.
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -207,6 +209,36 @@ impl FileScanTaskReader {
arrow_metadata
};
+ let project_pos =
task.project_field_ids().contains(&RESERVED_FIELD_ID_POS);
Review Comment:
Reusing `RowNumber` instead of a manual counter is the right, idiomatic
call. It inherits the parquet reader's correctness for selection and row-group
boundaries and matches Java's `SupportsRowPosition` semantics. Nice.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -240,11 +242,18 @@ impl RecordBatchTransformerBuilder {
Ok(self)
}
+ /// Set virtual fields such as '_pos'
+ pub(crate) fn with_virtual_field(mut self, field_id: i32) -> Self {
Review Comment:
The setter accepts any `field_id`, but it only works if a matching arrow-rs
virtual column was registered on the reader; otherwise it surfaces as the
runtime error you added in `generate_transform_operations`. A doc line ("caller
must have registered a corresponding
`ArrowReaderOptions::with_virtual_columns`") would make the coupling explicit.
--
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]
