Repository navigation
fix: skip dynamic filter pushdown for null-aware anti joins with a nullable build key - #23173
Conversation
|
@mdashti |
|
@mdashti Can you resolve the CI failures when you get a chance? |
The pushed filter can empty the probe. A null-aware `LeftAnti` then reads it as an empty subquery and emits build-side NULLs that should be dropped. A NOT NULL build key is unaffected.
Indexing on[0] leaned on the single-key invariant without saying so. Iterating all keys survives a future multi-key relaxation. In addition, added allow/reject unit tests to pin both directions of the guard.
The guard removes the probe-side dynamic filter from these plans.
32ead3e to
e512368
Compare
|
@kosiew @neilconway the failures were stale |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23173 +/- ##
==========================================
- Coverage 80.70% 80.70% -0.01%
==========================================
Files 1089 1089
Lines 368038 368096 +58
Branches 368038 368096 +58
==========================================
+ Hits 297031 297067 +36
- Misses 53308 53325 +17
- Partials 17699 17704 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
run benchmark tpch |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing moe/null-aware-build-null (e512368) to 4184b07 (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
| return false; | ||
| } | ||
|
|
||
| // A null-aware anti join emits a build-side NULL only when the probe |
There was a problem hiding this comment.
the comment says anto-join but the check isn't for join type. I wonder if that is a discrepancy?
…llable build key (apache#23173) ## Which issue does this close? - Closes apache#23126. ## Rationale for this change `x NOT IN (subquery)` plans to a null-aware `LeftAnti` hash join (build = outer `x`, probe = subquery). Join dynamic filter pushdown pushes a bounds + membership filter, built from the build keys, onto the probe scan. That filter can prune every probe row. A null-aware `LeftAnti` reads an empty probe as a genuinely-empty subquery, so it emits build-side NULL rows that should drop: `NULL NOT IN (non-empty)` is UNKNOWN, not TRUE. The result is scan-dependent, so it's a silent correctness bug. A `VALUES` scan ignores the pushed filter and stays correct; a parquet scan applies it and is wrong. apache#23103 (the probe-side NULL drop) is orthogonal; this is the build-side NULL. ## What changes are included in this PR? Skip join dynamic filter pushdown for a null-aware anti join when the build key can be NULL. The build-side NULL emission depends on whether the probe is truly empty, which the pushed filter can change by emptying it. A NOT NULL build key has no such NULL, so it keeps the pushdown. The check is static: a schema-nullable build key disables the pushdown even when the data contains no NULLs. A runtime alternative (keep the pushdown and neutralize the filter only when the build actually holds a NULL key) would restore the optimization for those cases. I'd leave that as a follow-up. ## Are these changes tested? Yes. A `push_down_filter_parquet.slt` case reproduces it (build-side NULL, a non-matching parquet probe) and asserts the single correct row. Without the change it returns the extra NULL. In addition, unit tests pin both directions of the guard: a nullable build key rejects the pushdown and a NOT NULL build key keeps it. ## Are there any user-facing changes? `NOT IN` over a parquet (or otherwise prunable) scan with a nullable outer key now returns correct results. Such joins lose the dynamic filter pushdown.
apache#24045) ## Which issue does this PR close? - Closes apache#24002. ## Rationale for this change Pushing filters to an anti join's non-output side can produce incorrect results. ## What changes are included in this PR? Restrict anti-join filter pushdown to the output side while preserving two-sided join-key pushdown for semi joins. ## Are these changes tested? Yes, with updated unit and SQL logic tests. ## Are there any user-facing changes? no API changes but some downstream expected plans may change (cherry picked from commit a3f0f93) Conflict resolved when applying to spiceai-54 (DataFusion 54.1): - datafusion/sqllogictest/test_files/dynamic_filter_pushdown_config.slt: the expected physical plan for the NOT IN + TopK query keeps the 54.1 display (no null_aware marker on HashJoinExec, no dynamic_rg_pruning field). With this change the TopK filter no longer reaches the non-output side, so the right-side scan drops from two dynamic filters to one; the one that remains is the hash join's own probe-side filter, which upstream no longer pushes for a null-aware anti join with a nullable build key (5975a2d, apache#23173, not on 54.1). Query results are unchanged from upstream.
…llable build key (apache#23173) ## Which issue does this close? - Closes apache#23126. ## Rationale for this change `x NOT IN (subquery)` plans to a null-aware `LeftAnti` hash join (build = outer `x`, probe = subquery). Join dynamic filter pushdown pushes a bounds + membership filter, built from the build keys, onto the probe scan. That filter can prune every probe row. A null-aware `LeftAnti` reads an empty probe as a genuinely-empty subquery, so it emits build-side NULL rows that should drop: `NULL NOT IN (non-empty)` is UNKNOWN, not TRUE. The result is scan-dependent, so it's a silent correctness bug. A `VALUES` scan ignores the pushed filter and stays correct; a parquet scan applies it and is wrong. apache#23103 (the probe-side NULL drop) is orthogonal; this is the build-side NULL. ## What changes are included in this PR? Skip join dynamic filter pushdown for a null-aware anti join when the build key can be NULL. The build-side NULL emission depends on whether the probe is truly empty, which the pushed filter can change by emptying it. A NOT NULL build key has no such NULL, so it keeps the pushdown. The check is static: a schema-nullable build key disables the pushdown even when the data contains no NULLs. A runtime alternative (keep the pushdown and neutralize the filter only when the build actually holds a NULL key) would restore the optimization for those cases. I'd leave that as a follow-up. ## Are these changes tested? Yes. A `push_down_filter_parquet.slt` case reproduces it (build-side NULL, a non-matching parquet probe) and asserts the single correct row. Without the change it returns the extra NULL. In addition, unit tests pin both directions of the guard: a nullable build key rejects the pushdown and a NOT NULL build key keeps it. ## Are there any user-facing changes? `NOT IN` over a parquet (or otherwise prunable) scan with a nullable outer key now returns correct results. Such joins lose the dynamic filter pushdown. (cherry picked from commit 5975a2d) Conflicts resolved when applying to spiceai-54 (DataFusion 54.1): - datafusion/physical-plan/src/joins/hash_join/exec.rs: added only this commit's null-aware gate to allow_join_dynamic_filter_pushdown and its two unit tests. The surrounding upstream context (the NullEqualsNull gate and its test from baa497d, apache#22965, and the swap-inputs and range-partitioning tests) is not on 54.1 and is not added. - datafusion/sqllogictest/test_files/dynamic_filter_pushdown_config.slt: kept the 54.1 plan display (no null_aware, no dynamic_rg_pruning) and removed the hash join's own dynamic filter from the probe scan of both NOT IN plans. For the NOT IN + TopK plan that leaves the right-side scan with no predicate: the TopK filter was already removed there by a3f0f93 (apache#24045), which is upstream's state once both changes are in. - datafusion/sqllogictest/test_files/explain_tree.slt: dropped. The change edits a NOT IN record that 127731b (apache#22913) added and 54.1 does not have. - datafusion/sqllogictest/test_files/push_down_filter_parquet.slt: added only this commit's regression block, not the null-equal join block from baa497d (apache#22965) that precedes it upstream.
## Which issue does this PR close? - Closes #apache#22964 ## Rationale for this change We presently allow dynamic filter pushdown to be applied to null-equal hash joins. This might result in pushing a predicate down into the probe-side plan, where the predicate will not be evaluated with the null-equal semantics that are required. Longer-term, we might consider supporting this case with the correct semantics (e.g., generate a predicate with `OR IS NULL ...`), but for now disabling pushdown for null-equal joins seems much more practical. ## What changes are included in this PR? * Disable hash join dynamic filter pushdown for null-equal joins * Add SLT test with end-to-end repro * Add unit test ## Are these changes tested? Yes. ## Are there any user-facing changes? No. (cherry picked from commit baa497d) Conflicts resolved when applying to spiceai-54 (DataFusion 54.1): - datafusion/physical-plan/src/joins/hash_join/exec.rs: this branch already carries 5975a2d (apache#23173), which upstream merged later and which adds its own gate and tests at the same places. Placed this commit's gate and test ahead of those, the order they have upstream. - datafusion/sqllogictest/test_files/push_down_filter_parquet.slt: same; the null-equal block goes ahead of the apache#23173 block, as upstream has it.
## Which issue does this PR close? - None. ## Rationale for this change `HashJoinExec` supports SQL `NOT IN` through `null_aware = true`, in three forms: `LeftAnti`, `LeftMark` and `RightAnti`, each correlated or not. The rules for which rows such a join emits are spread out today: - `exec.rs`: `NullAwareMode`, four `JoinLeftData` fields and one `JoinBuildData` field that only some forms use, and the correlated per-row state built inside the build function. - `stream.rs`: about 700 lines inside the regular join stream. These are the batch-skip decision, the correlated marking pass and its helpers, the RightAnti NULL and UNKNOWN drops, and two final-stage functions. - `utils.rs`: a second copy of the final rule, used for the LeftMark column. The two copies of the final rule already drifted once: LeftMark checked logical NULLs since apache#25693, LeftAnti physical ones until apache#26578. This code changes often. Since July it gained four features (apache#23957, apache#21585, apache#19857, apache#26213) and seven correctness fixes (apache#23104, apache#23173, apache#25076, apache#25339, apache#25693, apache#26586, apache#26578), and apache#26214, apache#25679 and apache#26088 propose more. Each such change has to find the `NOT IN` rules in three files, usually about six functions, interleaved with the regular join. A change to the regular probe loop has to read past them too. This PR moves the rules into `hash_join/not_in.rs`, with one copy of the final rule, so a `NOT IN` change touches one module and a regular-join change doesn't need to read it. The join reaches it at four points: 1. `NotIn::try_new` decides when the plan is built or executed whether `null_aware` is legal for the join, and which form it is: `BuildRows { scope, output: Rows | Mark }` for LeftAnti and LeftMark, or `ProbeRows { scope }` for RightAnti, where `scope` is `Uncorrelated` or `Correlated`. 2. `NotInBuild::try_new` runs once per build. It has one variant per form, holding only that form's state. `JoinLeftData` keeps a single `Option<NotInBuild>`. 3. `NotInProbe::before_lookup` / `after_lookup` run around each probe batch's lookup, and `after_empty_build` replaces `after_lookup` when the build map is empty. They skip the batch, record what the probe saw, mark build rows or this batch's probe rows UNKNOWN, and drop the probe rows a RightAnti must not emit; for other joins they do nothing. 4. `NotInBuild::final_chunk` runs on the build rows emitted at the end. It applies one rule for an unmatched row's mark, used both by LeftAnti (keep the rows whose mark is FALSE) and by LeftMark (the mark column). ## What changes are included in this PR? Six commits, each compiling and passing clippy on its own: 1. **Move the correlated marking helpers into `not_in.rs`.** The code is unchanged, apart from one signature that now takes the build data and probe batch instead of the stream's private state structs. This commit also adds a test for correlated marking when every build value key is NULL. The full-key map is empty then, and the marking must still run before the empty-map shortcut; nothing covered this before. 2. **`NotIn` replaces `NullAwareMode`.** The legality checks, messages and order are the same. 3. **`NotInBuild` holds the per-form build state, with one final rule.** It replaces the `JoinLeftData` / `JoinBuildData` fields, the two final-stage functions in `stream.rs`, and `build_null_aware_left_mark_column` in `utils.rs`. 4. **The `NotInProbe` hooks replace the stream's `NOT IN` branches and fields.** The correlated RightAnti's per-batch UNKNOWN probe rows move from `ProcessProbeBatchState` into `NotInProbe`, which recomputes them on each batch's first lookup. 5. **Review follow-ups.** `HashJoinExec::not_in()` and the `not_in` parameters replace `null_aware_mode()` and `null_aware: Option<NotIn>`, the helpers used only inside `not_in.rs` become private, and `final_chunk` asserts that a probe side with a NULL key is not empty. 6. **Test the correlated RightAnti empty-build path.** `test_null_aware_correlated_all_null_build_values` also runs RightAnti, which must drop the UNKNOWN probe rows when the build map is empty (`NotInProbe::after_empty_build`). Types: `NullAwareMode` goes, and `NotIn`, `NotInScope`, `NotInOutput`, `NotInBuild` and `NotInProbe` come in. Gone: the `NOT IN` fields of `JoinLeftData`, `JoinBuildData`, `HashJoinStream` and `ProcessProbeBatchState` and their constructor parameters, 4 stream functions (`null_aware_skip_probe_batch`, `drop_null_probe_keys`, `null_aware_left_anti_final_indices`, `null_aware_left_mark_column`), and the second copy of the final rule. What does not change: - the planner rules, including the `JoinSelection` swap to RightAnti; - the dynamic-filter gate for null-aware joins; - the prefilter and prepared-build exclusions; - the cross-partition `ProbeCompletion`. ### Behavior notes No query results change. Two internal orders do: - **Reservation order.** For an uncorrelated LeftAnti or LeftMark join whose build key has newly materialized logical NULLs (a dictionary, REE or Null key), the NULL buffer is now charged to the memory reservation right after the visited bitmap. On main it was charged after the membership step's optional pruning bitmap. Under a tight memory limit, a different allocation may be the one that fails. The pruning bitmap only exists with a dynamic filter, which a null-aware join with a nullable build key doesn't get. - **The form is computed twice per execution.** The stream reads it from the build (`JoinLeftData::not_in`); `execute` derives it again (`HashJoinExec::not_in()`) for the dynamic filter. Main derived it three times (build, stream, accumulator). The two can only disagree if an executed plan is rebuilt and run again without `reset_state`, which is already unsupported. Main panicked in that case; this PR uses the build's form. ## Performance Benchmark bot (GKE `c4a-highmem-16`, aarch64) on `a9d4896d7e` against its merge base `702803d022`; the later commit only adds a test: - `null_aware_join`: geomean 0.989. Q01 and Q03 are 4% faster, Q02, Q05 and Q06 about 2% faster, Q08 2% slower (20.4 → 20.8 ms), the rest are within 1%. - `hj`: total 6870 → 6885 ms (+0.2%). Q22 and Q24 are unchanged. The queries the bot flags (Q11, Q14, Q15, Q17 slower; Q16, Q18 faster) move within their run-to-run spread. - `tpch10`: all 22 queries unchanged, total 7078 → 7049 ms. Earlier runs on an M-series Mac showed this refactor slowing `hj` Q24 by up to 9% through different LLVM inlining of the probe loop. That doesn't reproduce on the bot, so the PR leaves inlining to the compiler. Not measured on x86_64. ## What is the testing strategy for this PR? - New test: `test_null_aware_correlated_all_null_build_values`, covering LeftAnti, LeftMark and RightAnti with the probe in two batches, at every batch-size config. - Existing tests: - physical-plan unit tests; - the null-aware join fuzz test (`null_aware_join_fuzz`, with `extended_tests`), which covers uncorrelated and correlated RightAnti plans after the `JoinSelection` swap; - the `null_aware_*` and `subquery*` sqllogictest files, with no changes to expected results or `EXPLAIN` output. ## Are there any user-facing changes? No.
Which issue does this close?
NOT IN+ dynamic filters + build-side NULL returns incorrect results #23126.Rationale for this change
x NOT IN (subquery)plans to a null-awareLeftAntihash join (build = outerx, probe = subquery). Join dynamic filter pushdown pushes a bounds + membership filter, built from the build keys, onto the probe scan. That filter can prune every probe row. A null-awareLeftAntireads an empty probe as a genuinely-empty subquery, so it emits build-side NULL rows that should drop:NULL NOT IN (non-empty)is UNKNOWN, not TRUE.The result is scan-dependent, so it's a silent correctness bug. A
VALUESscan ignores the pushed filter and stays correct; a parquet scan applies it and is wrong.#23103 (the probe-side NULL drop) is orthogonal; this is the build-side NULL.
What changes are included in this PR?
Skip join dynamic filter pushdown for a null-aware anti join when the build key can be NULL. The build-side NULL emission depends on whether the probe is truly empty, which the pushed filter can change by emptying it. A NOT NULL build key has no such NULL, so it keeps the pushdown.
The check is static: a schema-nullable build key disables the pushdown even when the data contains no NULLs. A runtime alternative (keep the pushdown and neutralize the filter only when the build actually holds a NULL key) would restore the optimization for those cases. I'd leave that as a follow-up.
Are these changes tested?
Yes. A
push_down_filter_parquet.sltcase reproduces it (build-side NULL, a non-matching parquet probe) and asserts the single correct row. Without the change it returns the extra NULL. In addition, unit tests pin both directions of the guard: a nullable build key rejects the pushdown and a NOT NULL build key keeps it.Are there any user-facing changes?
NOT INover a parquet (or otherwise prunable) scan with a nullable outer key now returns correct results. Such joins lose the dynamic filter pushdown.