mbutrovich opened a new issue, #3355: URL: https://github.com/apache/iceberg-rust/issues/3355
## Apache Iceberg Rust version main (ecba0b537a1f78ec8125c1a26f6dfab3f95b509e) ## Describe the bug The spec's [column projection rules](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/format/spec.md?plain=1#L398-L408) say that a reader resolves a field ID that isn't in a data file to null when the field has no identity partition value and no `initial-default`. That happens for any data file written before an optional column was added without a default. `PredicateConverter` in `predicate_visitor.rs` builds the Arrow row filter, and it handles a column missing from the file with one constant per operator. [`less_than` and `less_than_or_eq`](https://github.com/apache/iceberg-rust/blob/ecba0b537a1f78ec8125c1a26f6dfab3f95b509e/crates/iceberg/src/arrow/reader/predicate_visitor.rs#L420-L457) return `build_always_true()`, so `b < 5` keeps every row of such a file. [`greater_than`, `greater_than_or_eq`, `eq`, and `not_eq`](https://github.com/apache/iceberg-rust/blob/ecba0b537a1f78ec8125c1a26f6dfab3f95b509e/crates/iceberg/src/arrow/reader/predicate_visitor.rs#L460-L537) return `build_always_false()`. A file that stores `b` as nulls gives the opposite answer for `<` and `<=`. The Arrow comparison kernels return null for a null input, and the row filter drops the row. [`ExpressionEvaluator`](https://github.com/apache/iceberg-rust/blob/ecba0b537a1f78ec8125c1a26f6dfab3f95b509e/crates/iceberg/src/expr/visitors/expression_evaluator.rs#L127-L137) also returns false for `<` on a null value. So the rows a scan returns depend on whether a null column is stored in the file or missing from it, and the missing case is a silent wrong answer. #3298 says that when neither value exists, the current null handling is correct. That is wrong for `<` and `<=`. @comphead pointed this out in [review of #3302](https://github.com/apache/iceberg-rust/pull/3302#discussion_r4139977182). In Iceberg Java, the row-level `Evaluator` compares with a [nulls-first comparator](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/api/src/main/java/org/apache/iceberg/expressions/Literals.java#L160-L174), so [`lt` and `ltEq`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/api/src/main/java/org/apache/iceberg/expressions/Evaluator.java#L105-L114) are true for a null value there. The Parquet row group filter treats a missing column as all nulls instead, and [`TestMetricsRowGroupFilter.testColumnNotInFile`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/data/src/test/java/org/apache/iceberg/data/TestMetricsRowGroupFilter.java#L445-L473) expects `lessThan` and `lessThanOrEqual` to skip the file. ## To Reproduce The test below fails on main. It goes in the `tests` module of `crates/iceberg/src/arrow/reader/row_filter.rs` and uses its `field_with_id`, `write_row_groups`, and `read_once` helpers. It needs no new imports. ```rust /// Both files read `b` as null on every row. One doesn't store `b`, and the other stores it /// as nulls. #[tokio::test] async fn test_comparison_on_missing_column_matches_stored_nulls() { 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, "b", Type::Primitive(PrimitiveType::Long)).into(), ]) .build() .unwrap(), ); let tmp_dir = TempDir::new().unwrap(); let a = Arc::new(Int64Array::from(vec![1, 2, 3])) as ArrayRef; let missing_path = format!("{}/missing.parquet", tmp_dir.path().to_str().unwrap()); let missing_schema = Arc::new(ArrowSchema::new(vec![field_with_id( "a", DataType::Int64, 1, )])); let batch = RecordBatch::try_new(missing_schema.clone(), vec![a.clone()]).unwrap(); write_row_groups(&missing_path, missing_schema, vec![batch], false); let nulls_path = format!("{}/nulls.parquet", tmp_dir.path().to_str().unwrap()); let b_field = Field::new("b", DataType::Int64, true).with_metadata(HashMap::from([( PARQUET_FIELD_ID_META_KEY.to_string(), "2".to_string(), )])); let nulls_schema = Arc::new(ArrowSchema::new(vec![ field_with_id("a", DataType::Int64, 1), b_field, ])); let batch = RecordBatch::try_new(nulls_schema.clone(), vec![ a, Arc::new(Int64Array::new_null(3)), ]) .unwrap(); write_row_groups(&nulls_path, nulls_schema, vec![batch], false); let b = || Reference::new("b"); let mut actual = Vec::new(); for predicate in [ b().less_than(Datum::long(5)), b().less_than_or_equal_to(Datum::long(5)), b().greater_than(Datum::long(5)), b().greater_than_or_equal_to(Datum::long(5)), ] { let mut rows = Vec::new(); for path in [&missing_path, &nulls_path] { let (batches, _) = read_once(path, schema.clone(), vec![1, 2], predicate.clone(), false).await; rows.push(batches.iter().map(RecordBatch::num_rows).sum::<usize>()); } actual.push((predicate.to_string(), rows)); } let expected: Vec<_> = actual .iter() .map(|(predicate, _)| (predicate.clone(), vec![0, 0])) .collect(); assert_eq!(actual, expected); } ``` Running `cargo test -p iceberg --lib -- test_comparison_on_missing_column_matches_stored_nulls` on main gives the following. Each pair is the number of rows kept from the file without `b` and from the file with null `b`. ``` left: [("b < 5", [3, 0]), ("b <= 5", [3, 0]), ("b > 5", [0, 0]), ("b >= 5", [0, 0])] right: [("b < 5", [0, 0]), ("b <= 5", [0, 0]), ("b > 5", [0, 0]), ("b >= 5", [0, 0])] ``` ## Expected behavior `b < 5` and `b <= 5` should keep no rows of a file that doesn't store `b`, the same as for a file that stores `b` as nulls. That means `less_than` and `less_than_or_eq` in `PredicateConverter` should return `build_always_false()` for a missing column. ## 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]
