mbutrovich commented on code in PR #2695:
URL: https://github.com/apache/iceberg-rust/pull/2695#discussion_r3500989818
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -862,7 +863,7 @@ pub mod tests {
.file_format(DataFileFormat::Parquet)
.file_size_in_bytes(parquet_file_size)
.record_count(1)
-
.partition(Struct::from_iter([Some(Literal::long(100))]))
+
.partition(Struct::from_iter([Some(Literal::long(1))]))
Review Comment:
Collapsing three distinct partition values to `1/1/1` reduces distinctness
for every test on `setup_manifest_files` and forces the `inspect/manifests.rs`
bound edits below. Was there a failure with the original values that motivated
this? If the new constant-type handling needs specific values, a separate
fixture might keep the shared one intact. But if the old values actually broke,
that itself sounds like a test worth keeping. What did you hit here?
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -2369,4 +2368,114 @@ pub mod tests {
// Assert it finished (didn't timeout)
assert!(result.is_ok(), "Scan timed out - deadlock detected");
}
+
+ #[tokio::test]
Review Comment:
Both new tests assert `0` on a single-spec table. The whole point of
`_spec_id` (https://iceberg.apache.org/spec/#reserved-field-ids) is
distinguishing files written under different specs. A partition-evolved fixture
where two files carry different spec ids would validate that directly. Would
that be easy to add on top of the existing fixtures?
##########
crates/iceberg/src/inspect/manifests.rs:
##########
@@ -366,12 +366,12 @@ mod tests {
-- child 2: "lower_bound" (Utf8)
StringArray
[
- "100",
+ "1",
Review Comment:
This expected-output edit only exists because the fixture changed; if the
fixture stays, this revert isn't needed. Flagging so reviewers know it's
coupled to the above, not an independent behavior change.
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -248,6 +250,16 @@ impl FileScanTaskReader {
record_batch_transformer_builder.with_constant(RESERVED_FIELD_ID_FILE,
file_datum);
}
+ if task
+ .project_field_ids()
+ .contains(&RESERVED_FIELD_ID_SPEC_ID)
+ && let Some(partition_spec) = &task.partition_spec
+ {
+ let spec_id_datum = Datum::int(partition_spec.spec_id());
+ record_batch_transformer_builder = record_batch_transformer_builder
Review Comment:
Routing `_spec_id` through `with_constant` makes it RunEndEncoded
(metadata-field constants get `datum_to_arrow_type_with_ree`), same as `_file`.
The spec types `_spec_id` as a plain `int`
(https://iceberg.apache.org/spec/#reserved-field-ids). For the Comet/Spark
consumer, does REE get decoded cheaply downstream, or would a plain `Int32`
avoid a cast? Not necessarily a change, just confirming the consumer is happy
with REE here as it is for `_file`.
##########
crates/iceberg/src/arrow/value.rs:
##########
@@ -969,7 +972,7 @@ pub(crate) fn create_primitive_array_repeated(
(dt, _) => {
return Err(Error::new(
ErrorKind::Unexpected,
- format!("unexpected target column type {dt}"),
+ format!("unexpected target column type {dt}, prim_lit {:?}",
prim_lit),
Review Comment:
Mixed styles in one string; `...{prim_lit:?}` matches the inline-arg
convention used right above it.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -486,7 +482,20 @@ impl RecordBatchTransformer {
// they exist in the Parquet file. This is per Iceberg spec
rule #1: partition metadata
// is authoritative and should be preferred over file data.
if let Some(datum) = constant_fields.get(field_id) {
- let arrow_type = datum_to_arrow_type_with_ree(datum);
+ let arrow_type = if get_metadata_field(*field_id).is_ok() {
+ datum_to_arrow_type_with_ree(datum)
+ } else {
+ field_id_to_mapped_schema_map
+ .get(field_id)
Review Comment:
Separating "metadata field becomes REE" from "identity-partition field
becomes schema type" looks like a real correctness improvement:
identity-partition constants shouldn't be forced to RunEndEncoded. Worth
calling out to the #2668/#2746 authors so they rebase on top rather than
reintroducing the old derivation. This branch is the scalar arm that #2668
would fold into a `MetadataColumnSource::ScalarConstant`; keeping it in this
shape makes that easy.
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -248,6 +250,16 @@ impl FileScanTaskReader {
record_batch_transformer_builder.with_constant(RESERVED_FIELD_ID_FILE,
file_datum);
}
+ if task
+ .project_field_ids()
+ .contains(&RESERVED_FIELD_ID_SPEC_ID)
+ && let Some(partition_spec) = &task.partition_spec
Review Comment:
@advancedxy already raised this; this is a +1 to second it, to post as a
reply on that thread:
"+1. If the invariant is that `task.partition_spec` is always present
(spec_id = 0 even for unpartitioned tables), then gating the column on `let
Some(partition_spec)` means a projected `_spec_id` can silently produce no
column when that invariant is ever violated, which would be a confusing 'column
vanished' bug rather than a clean error. Adding `_spec_id` to the constant map
whenever it's projected (per @advancedxy) is the safer contract. Worth a test
on an unpartitioned table to lock in spec_id = 0."
--
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]
