mbutrovich opened a new issue, #3301: URL: https://github.com/apache/iceberg-rust/issues/3301
## Apache Iceberg Rust version main (deab0692f6df211a396d6fc0ad21811f8b6c8491) ## Describe the bug The spec's [column projection rules](https://github.com/apache/iceberg/blob/48330b8dacab6662242d252b39c8444190979bb2/format/spec.md?plain=1#L403-L408) resolve a field ID that isn't in a data file in order. The [first rule](https://github.com/apache/iceberg/blob/48330b8dacab6662242d252b39c8444190979bb2/format/spec.md?plain=1#L405) returns the value from the file's `partition` struct if the field has an [identity transform](https://github.com/apache/iceberg/blob/48330b8dacab6662242d252b39c8444190979bb2/format/spec.md?plain=1#L573). Only if that rule doesn't apply does the reader [return `initial-default`](https://github.com/apache/iceberg/blob/48330b8dacab6662242d252b39c8444190979bb2/format/spec.md?plain=1#L407), and then [null](https://github.com/apache/iceberg/blob/48330b8dacab6662242d252b39c8444190979bb2/format/spec.md?plain=1#L408). A null identity partition value is a value, not a missing one. [Partition values must be the same for all records in a data file](https://github.com/apache/iceberg/blob/48330b8dacab6662242d252b39c8444190979bb2/format/spec.md?plain=1#L550), identity returns its source value unmodified, and [every transform returns null only for a null input](https://github.com/apache/iceberg/blob/48330b8dacab6662242d252b39c8444190979bb2/format/spec.md?plain=1#L582). So a null identity partition value means every row in the file has null in that column. As I read the spec, the first rule applies and the reader should return null. `RecordBatchTransformer` doesn't do that. [`constants_map` skips null partition values](https://github.com/apache/iceberg-rust/blob/deab0692f6df211a396d6fc0ad21811f8b6c8491/crates/iceberg/src/arrow/record_batch_transformer.rs#L108-L114), so the column falls through to the later rules and [reads as `initial-default`](https://github.com/apache/iceberg-rust/blob/deab0692f6df211a396d6fc0ad21811f8b6c8491/crates/iceberg/src/arrow/record_batch_transformer.rs#L852-L878) when the field has one. The comment on the skip says the value "will be resolved as null per Iceberg spec rule #4", but rule 3 (`initial-default`) comes first. The result is a silent wrong answer: rows that hold null read back as the default. Iceberg Java returns null. [`PartitionUtil.constantsMap`](https://github.com/apache/iceberg/blob/48330b8dacab6662242d252b39c8444190979bb2/core/src/main/java/org/apache/iceberg/util/PartitionUtil.java#L92-L100) stores null identity partition values. [`ParquetValueReaders.replaceWithMetadataReader`](https://github.com/apache/iceberg/blob/48330b8dacab6662242d252b39c8444190979bb2/parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java#L226-L228) checks `idToConstant.containsKey(id)` ("containsKey is used because the constant may be null") and returns a constant reader for the null. [`defaultReader`](https://github.com/apache/iceberg/blob/48330b8dacab6662242d252b39c8444190979bb2/parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java#L272-L287) only falls back to `initialDefault()` when no reader was found, so the default is never used. This needs a file that doesn't store its identity partition column, as after a Hive migration or `add_files`, a null partition value, and an `initial-default` on that column. I found it while working on #3298. The fix for #3298 resolves filter values through the same `constants_map`, so filtering reads the same wrong value that projection does, and fixing `constants_map` fixes both. ## To Reproduce The test below fails on main. It goes in the `tests` module of `crates/iceberg/src/arrow/reader/row_filter.rs` and reuses its `field_with_id` and `write_row_groups` helpers. ```rust /// A file that doesn't store its identity partition column `p`, as after a Hive migration /// or `add_files`, and whose partition value for `p` is null. Every row reads `p` as the /// partition value null, not as `p`'s `initial-default` 7. #[tokio::test] async fn test_null_identity_partition_value_overrides_initial_default() { use arrow_array::types::Int64Type; use crate::spec::{Literal, PartitionSpec, Struct, Transform}; let schema = Arc::new( Schema::builder() .with_schema_id(1) .with_fields(vec![ NestedField::required(1, "a", Type::Primitive(PrimitiveType::Long)).into(), NestedField::optional(2, "p", Type::Primitive(PrimitiveType::Long)) .with_initial_default(Literal::long(7)) .into(), ]) .build() .unwrap(), ); let partition_spec = Arc::new( PartitionSpec::builder(schema.clone()) .add_partition_field("p", "p", Transform::Identity) .unwrap() .build() .unwrap(), ); let tmp_dir = TempDir::new().unwrap(); let file_path = format!("{}/1.parquet", tmp_dir.path().to_str().unwrap()); let arrow_schema = Arc::new(ArrowSchema::new(vec![field_with_id( "a", DataType::Int64, 1, )])); let batch = RecordBatch::try_new(arrow_schema.clone(), vec![Arc::new(Int64Array::from( vec![1, 2, 3], ))]) .unwrap(); write_row_groups(&file_path, arrow_schema, vec![batch], false); let task = FileScanTask::builder() .with_file_size_in_bytes(std::fs::metadata(&file_path).unwrap().len()) .with_start(0) .with_length(0) .with_data_file_path(file_path.clone()) .with_data_file_format(DataFileFormat::Parquet) .with_schema(schema.clone()) .with_project_field_ids(vec![1, 2]) .with_partition_spec(Some(partition_spec)) .with_partition(Some(Struct::from_iter([None]))) .with_case_sensitive(false) .build() .unwrap(); let tasks = Box::pin(futures::stream::iter(vec![Ok(task)])) as FileScanTaskStream; let batches: Vec<RecordBatch> = ArrowReaderBuilder::new(FileIO::new_with_fs(), Runtime::current()) .build() .read(tasks) .unwrap() .stream() .try_collect() .await .unwrap(); let p = batches[0].column(1).as_primitive::<Int64Type>(); assert_eq!(p.iter().collect::<Vec<_>>(), vec![None, None, None]); } ``` Running `cargo test -p iceberg --lib -- test_null_identity_partition_value_overrides_initial_default` on main gives: ``` assertion `left == right` failed left: [Some(7), Some(7), Some(7)] right: [None, None, None] ``` ## Expected behavior When a field has an identity transform in the file's partition spec, the column is missing from the data file, and the partition value is null, every row should read the field as null, even if the field has an `initial-default`. `constants_map` returns `HashMap<i32, Datum>`, and a `Datum` can't hold null, so the fix needs another way to register a null constant for these fields. [`ColumnConstant::Null`](https://github.com/apache/iceberg-rust/blob/deab0692f6df211a396d6fc0ad21811f8b6c8491/crates/iceberg/src/arrow/record_batch_transformer.rs#L257-L263) already represents an all-null column for metadata columns. ## Willingness to contribute I can contribute a fix for this bug independently -- 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]
