Repository navigation
fix(arrow): evaluate filters on columns missing from a data file against their identity partition value or initial-default - #3302
mbutrovich wants to merge 9 commits into
Conversation
…nst their identity partition value or initial-default
comphead
left a comment
There was a problem hiding this comment.
I compared the expected results with Java's TestMetricsRowGroupFilter and TestFilterPushDown, and they match. The inline comments cover the main points. Smaller nits:
field_idsinresidual_for_missing_fieldsandMissingFieldResidualVisitoronly holds the missing fields that resolved to a value. A name likeconstant_field_idswould match theconstants_mapwording and set it apart frompredicate_field_ids.- A one-line comment at the
residual_for_missing_fieldscall inpipeline.rswould help. It should say that the residual runs before the row filter and every pruning layer so they all see the same predicate. constants_mapnow runs twice per file when the file has partition data and a predicate column is missing (once inwith_partition, once in the residual). It is cheap, so fine to leave.- The
readclosure inassert_absent_b_reads_as_7repeats the task and reader setup inread_once, which has two callers. Apartition: Option<(Arc<PartitionSpec>, Struct)>parameter onread_oncewould let the helper call it directly. The helper also buildsactualandexpectedin separate passes and callsto_string()twice per case. A single loop withassert_eq!(.., "{predicate}")would do the same in one pass.
| Some(datum) => Some(Literal::Primitive(datum.literal().clone())), | ||
| None => field.initial_default.clone(), | ||
| }; | ||
| if value.is_some() { |
There was a problem hiding this comment.
Question, not for this PR. The null handling that leaves on a field with no value fall back to is not uniform. In PredicateConverter, less_than and less_than_or_eq return build_always_true() for a missing column, while greater_than, greater_than_or_eq, and eq return false. So for a column added without a default, WHERE d < 5 keeps every row of an older file and WHERE d > 5 keeps none. Java's ParquetMetricsRowGroupFilter treats a missing column as all null, and TestMetricsRowGroupFilter.testColumnNotInFile expects lt and le to be unable to match. The issue calls the current null handling correct, so I wanted to check whether the < and <= branches are intended. If not, a separate issue would be enough.
There was a problem hiding this comment.
I don't think they're intended. I checked on this branch with an optional b that has no default. For a file that doesn't store b, b < 5 and b <= 5 keep all 3 rows, and b > 5, b >= 5, b = 5, and b != 5 keep none. For a file that stores b as 3 nulls, b < 5 and b <= 5 keep none. So the result depends on whether the nulls are stored or come from a missing column. The branches date back to #295, and I don't know why < and <= differ there. Java's row-level Evaluator compares with a nulls-first comparator, so lt is true for a null value there. That could be the source, but testColumnNotInFile expects the Parquet row group filter to skip the file for both. I opened #3355 for it.
# Conflicts: # crates/iceberg/src/scan/mod.rs
|
Thanks for the review, @comphead. I merged
Renamed to
Added.
I left it as is.
|
comphead
left a comment
There was a problem hiding this comment.
The earlier review points are addressed. The residual matches Java's ParquetMetricsRowGroupFilter.predicate for initial-default (top-level only) and ResidualEvaluator for identity partition values. CI is green and the branch merges cleanly with main.
I ran seven single-line production mutations against the new tests. Details are on the test comments below.
| _ => HashMap::new(), | ||
| }; | ||
|
|
||
| let mut constant_field_ids = HashSet::new(); |
There was a problem hiding this comment.
Nit: this set holds at most predicate_field_ids.len() entries, so HashSet::with_capacity(predicate_field_ids.len()) bounds it.
There was a problem hiding this comment.
Nit: this set holds at most
predicate_field_ids.len()entries, soHashSet::with_capacity(predicate_field_ids.len())bounds it.
I'd like to keep HashSet::new(), because it doesn't allocate until the first insert and with_capacity allocates up front. A common case here is a column added without a default. In that case no missing field has a value, and we return early with the set still empty, so with_capacity would allocate for nothing. When a field does have a value, the first allocation from new() holds three entries, which covers most predicates.
| struct ExpressionEvaluatorVisitor<'a> { | ||
| pub(crate) struct ExpressionEvaluatorVisitor<'a> { | ||
| /// Reference to a [`DataFile`]'s partition [`Struct`]. | ||
| partition: &'a Struct, |
There was a problem hiding this comment.
Nit: the struct doc and this field name still say "partition", but the residual now passes a row of top-level field values. Reusing the visitor is the right call. Updating the doc and renaming the field to row (or values) would keep the name accurate.
There was a problem hiding this comment.
Nit: the struct doc and this field name still say "partition", but the residual now passes a row of top-level field values.
I renamed the field to row and rewrote the struct doc. The docs on ExpressionEvaluator still say partition, since that is what it evaluates.
| /// The first file is written before `c` is added with `initial-default` 'US', so its row | ||
| /// reads `c` as 'US'. The second file stores `c`. | ||
| #[tokio::test] | ||
| async fn test_scan_filter_on_initial_default_column_absent_from_file() { |
There was a problem hiding this comment.
Removing the residual_for_missing_fields call from pipeline.rs fails this test, but it also fails the two reader tests in row_filter.rs, and no other mutation I tried fails only this one. It repeats the reader-level initial-default matrix (c = 'US', IS NOT NULL, IS NULL, row selection off and on). What it adds is the update_schema plumbing, which this PR does not change. Consider trimming it to one or two predicates, or dropping it and keeping the Java mirror in the reader test.
There was a problem hiding this comment.
Consider trimming it to one or two predicates, or dropping it and keeping the Java mirror in the reader test.
I trimmed it to c = 'US' and c IS NULL with row selection off. I kept it because it is the only test that goes through update_schema and TableScan. Its doc now points to the reader tests for the other predicates and for row selection.
comphead
left a comment
There was a problem hiding this comment.
Thanks @mbutrovich couple of nits, but looks good overall
|
Thanks for the second pass, @comphead. I pushed changes for the inline comments and replied to each one below. |
Which issue does this PR close?
What changes are included in this PR?
residual_for_missing_fieldsinarrow/reader/predicate_visitor.rs. It replaces each predicate leaf on a top-level field that is missing from the data file withAlwaysTrueorAlwaysFalse, evaluated against the value projection returns for that field: the identity partition value, otherwiseinitial-default. Leaves on missing fields with neither keep the existing null handling. This is the same idea as Java'sResidualEvaluator, extended toinitial-default. Like Java's, it folds the constants out ofANDandOR, so the row filter doesn't evaluate them per batch.ExpressionEvaluatorVisitorfor leaf evaluation andconstants_mapfor identity partition values. Both go from private topub(crate). There are no public API changes.Are these changes tested?
Yes, with unit tests, reader tests, and a
MemoryCatalogscan test:row_filter.rsread a file that doesn't storeb. In one,bcomes frominitial-default. In the other, it comes from an identity partition value. Each runs filters onband equality deletes onb(for 7, another value, and null), with row selection off and on. Both fail without the fix.predicate_visitor.rsfollow Java'sTestMetricsRowGroupFiltercases for string and double defaults. Other cases check that the partition value takes precedence overinitial-default, that bucket partition values are ignored, and that a present column, a column with no value, and a nested field are left unchanged. They also check that a constant on either side ofANDorORfolds away, that anANDwith no constant child is kept, thatNOTis pushed into its child, and that the keep predicates built from equality deletes fold to a single constant.test_scan_filter_on_initial_default_column_absent_from_fileinscan/mod.rsfollows Java'stestFilterPushdownOnInitialDefaultColumnAbsentFromFile. It appends a file, adds a column with a default throughupdate_schema, appends a second file, and scans withc = 'US'andc IS NULL. It fails without the fix.AI Disclosure
I wrote this PR with help from Claude but understand and support the core approach, implementation, and test coverage.