fix: treat typed nulls as missing bounds in aggregate dynamic filter merge - #25149
Open
uddhavdave wants to merge 3 commits into
Open
fix: treat typed nulls as missing bounds in aggregate dynamic filter merge#25149uddhavdave wants to merge 3 commits into
uddhavdave wants to merge 3 commits into
Conversation
…merge When aggregate dynamic filter pushdown is enabled with multiple partitions, a partition whose input lacks the aggregated column (for example a schema-evolved Parquet file) evaluates MIN/MAX to a typed null such as `Int64(NULL)`. The shared-bound merge only short-circuited on `ScalarValue::Null`, so the typed null fell through to `partial_cmp`, where `None` orders before `Some(_)`, and replaced a valid shared minimum. The dynamic filter then lost its lower bound and could prune the file holding the true MIN, returning a wrong result depending on partition scheduling. Use `ScalarValue::is_null()` so both untyped and typed nulls are ignored when merging bounds. Closes apache#25147
github-actions
Bot
added
sqllogictest
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
SELECT MIN(col), MAX(col)over a schema-evolved Parquet dataset can return a wrongMINwhendatafusion.optimizer.enable_aggregate_dynamic_filter_pushdownis enabled and the query runs with multiple partitions.When one of the files does not contain the aggregated column, the
Partialaggregate for that partition evaluates to a typed null such asInt64(NULL)rather thanScalarValue::Null. The merge of per-partition bounds into the shared dynamic filter bound only short-circuited onScalarValue::Null. The typed null therefore reachedpartial_cmp, whereNoneorders beforeSome(_), and either replaced a valid shared minimum or blocked a later valid minimum from being recorded. The dynamic filter then lost its lower bound and became justcol > <max>, which pruned the file holding the true minimum. The result depended on partition scheduling;target_partitions = 1or disabling the pushdown returned the correct answer.What changes are included in this PR?
scalar_cmp_null_short_circuitindatafusion/physical-plan/src/aggregates/aggregate_stream.rsnow usesScalarValue::is_null()so both untyped and typed nulls are treated as "no bound yet" when mergingMIN/MAXbounds across partitions.What is the testing strategy for this PR?
scalar_min_max_ignore_typed_nullsinaggregate_stream.rscoversscalar_min/scalar_maxwith typed nulls, untyped nulls, and regular values on either side. This fails without the fix and is deterministic.datafusion/sqllogictest/test_files/push_down_filter_regression.sltthat builds a three-file Parquet fixture (one file without the aggregated column) and runsMIN/MAXwithtarget_partitions = 8. The file holding the minimum is named to sort last so it is opened after the other partitions publish their bounds. With the fix reverted locally this reproduced the wrong result in 4 of 5 runs; with the fix it passes consistently. Because the failure depends on scheduling, the unit test is the primary regression guard and the slt case documents the end-to-end scenario.cargo fmt --all,cargo clippy --all-targets --all-features -- -D warnings, and thepush_down_filter_regressionsqllogictest all pass locally.Are there any user-facing changes?
No API changes. Queries affected by the bug now return the correct
MIN/MAX.