Repository navigation
Row filter keeps every row for < and <= on a null column that is missing from the data file #3355
Description
Apache Iceberg Rust version
main (ecba0b5)
Describe the bug
The spec's column projection rules 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 return build_always_true(), so b < 5 keeps every row of such a file. greater_than, greater_than_or_eq, eq, and not_eq 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 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.
In Iceberg Java, the row-level Evaluator compares with a nulls-first comparator, so lt and ltEq are true for a null value there. The Parquet row group filter treats a missing column as all nulls instead, and TestMetricsRowGroupFilter.testColumnNotInFile 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.
/// 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
Activity