You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "jayzhan211 (via GitHub)" <gi...@apache.org> on 2026/08/23 05:10:03 UTC
[PR] fix: hash join dynamic filter with dictionary-encoded join keys [datafusion]
jayzhan211 opened a new pull request, #24587:
URL: https://github.com/apache/datafusion/pull/24587
## Rationale for this change
Hash joins on a dictionary-encoded integer join key misbehave when dynamic filter pushdown is active. A dictionary array can carry NULL in its *values* while its key bitmap has no physical nulls (`null_count() == 0` but `logical_null_count() > 0`), and two places in the hash join only looked at the physical count:
1. **Wrong results:** for a `NullEqualsNull` join, `keys_have_null` was computed with `null_count()`, so the pushed dynamic filter was not widened with `key IS NULL` and pruned the probe-side NULL row that should have null-matched the build-side NULL.
2. **Query error:** `try_create_array_map` inferred "the key type is supported by `ArrayMap`" from the presence of min/max bounds. But bounds are also collected for dynamic filters on *any* key type, so with a dictionary key and a filter consumer the build side failed with `Unsupported type for ArrayMap: Dictionary(Int32, Int32)` instead of falling back to the regular hash map.
## What changes are included in this PR?
- `stream.rs`: compute `keys_have_null` with `logical_null_count()`.
- `exec.rs`: gate `ArrayMap` (perfect hash join) creation on the key type via `is_perfect_hash_join_candidate` rather than on the presence of bounds; also use `logical_null_count()` for the `NullEqualsNull` check in the same function.
- Rename `should_collect_min_max_for_perfect_hash` → `is_perfect_hash_join_candidate` with a doc comment, since it describes what is checked rather than a side effect.
## Are these changes tested?
Yes. A new test, `test_null_equal_dynamic_filter_keeps_probe_nulls_for_build_logical_null`, builds a `NullEqualsNull` inner join over dictionary keys with a dynamic filter consumer on the probe side and asserts both that the pushed filter contains the `IS NULL` disjunct and that the NULL–NULL row is produced. Before this PR it fails with the `ArrayMap` error; with only the `ArrayMap` gate it fails on the missing `IS NULL`. Existing hash join unit tests and the `joins`, `push_down_filter_parquet`, and `explain_analyze` sqllogictests pass.
## Are there any user-facing changes?
No API changes. Joins on dictionary-encoded integer keys with dynamic filter pushdown now return correct results instead of erroring / dropping rows.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: hash join dynamic filter with dictionary-encoded join keys [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #24587:
URL: https://github.com/apache/datafusion/pull/24587#issuecomment-5440568400
Sorry, I though auto-merge will be disabled if the test failed
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: hash join dynamic filter with dictionary-encoded join keys [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24587:
URL: https://github.com/apache/datafusion/pull/24587#issuecomment-5442997962
Seems it causes CI fail
- https://github.com/apache/datafusion/issues/24734
but that is probably what @jayzhan211 is referring to abve
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: hash join dynamic filter with dictionary-encoded join keys [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #24587:
URL: https://github.com/apache/datafusion/pull/24587
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: hash join dynamic filter with dictionary-encoded join keys [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24587:
URL: https://github.com/apache/datafusion/pull/24587#issuecomment-5384428864
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24587?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `83.33333%` with `11 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.38%. Comparing base ([`5134a1a`](https://app.codecov.io/gh/apache/datafusion/commit/5134a1adff1498e18cc3af34a79aaa62ccc4efb8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`71456db`](https://app.codecov.io/gh/apache/datafusion/commit/71456dbbb4721978a821e53fd557eb2f4653cd8f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24587?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...tafusion/physical-plan/src/joins/hash\_join/exec.rs](https://app.codecov.io/gh/apache/datafusion/pull/24587?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fjoins%2Fhash_join%2Fexec.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9qb2lucy9oYXNoX2pvaW4vZXhlYy5ycw==) | 83.07% | [1 Missing and 10 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24587?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24587 +/- ##
==========================================
- Coverage 81.38% 81.38% -0.01%
==========================================
Files 1116 1116
Lines 397960 398019 +59
Branches 397960 398019 +59
==========================================
+ Hits 323880 323922 +42
- Misses 55120 55127 +7
- Partials 18960 18970 +10
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24587?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: hash join dynamic filter with dictionary-encoded join keys [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #24587:
URL: https://github.com/apache/datafusion/pull/24587#discussion_r3871831054
##########
datafusion/physical-plan/src/joins/hash_join/stream.rs:
##########
@@ -559,11 +559,13 @@ impl HashJoinStream {
.bounds
.clone()
.unwrap_or_else(|| PartitionBounds::new(vec![]));
- // Arrow tracks null counts per array, so this costs no data scan.
+ // Use the logical null count: a dictionary key whose entry points at a
+ // NULL dictionary value is a NULL key even though the key bitmap has no
+ // physical nulls (`null_count() == 0` but `logical_null_count() > 0`).
let keys_have_null = left_data
.values()
.iter()
- .any(|array| array.null_count() > 0);
+ .any(|array| array.logical_null_count() > 0);
Review Comment:
Sure! Thanks your suggestion
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: hash join dynamic filter with dictionary-encoded join keys [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24587:
URL: https://github.com/apache/datafusion/pull/24587#discussion_r3868366601
##########
datafusion/physical-plan/src/joins/hash_join/stream.rs:
##########
@@ -559,11 +559,13 @@ impl HashJoinStream {
.bounds
.clone()
.unwrap_or_else(|| PartitionBounds::new(vec![]));
- // Arrow tracks null counts per array, so this costs no data scan.
+ // Use the logical null count: a dictionary key whose entry points at a
+ // NULL dictionary value is a NULL key even though the key bitmap has no
+ // physical nulls (`null_count() == 0` but `logical_null_count() > 0`).
let keys_have_null = left_data
.values()
.iter()
- .any(|array| array.null_count() > 0);
+ .any(|array| array.logical_null_count() > 0);
Review Comment:
Could we add the same dictionary-value logical-NULL regression coverage for `PartitionMode::Partitioned`, using multiple partitions? `keys_have_null` is also aggregated by `build_partitioned_filter`, while the new test currently exercises only `CollectLeft`. Covering the partitioned case would help catch regressions in how the logical NULL state is aggregated or routed across partitions.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org