Repository navigation
fix: hash join dynamic filter with dictionary-encoded join keys - #24587
Merged
jayzhan211 merged 2 commits intoAug 27, 2026
Merged
Conversation
- Use logical_null_count() for keys_have_null so a null-equal join's pushed filter keeps probe NULL rows when the build NULL lives in a dictionary value - Gate ArrayMap (perfect hash join) creation on the key type instead of the presence of bounds, which are also collected for dynamic filters on any type - Rename should_collect_min_max_for_perfect_hash -> is_perfect_hash_join_candidate
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24587 +/- ##
==========================================
+ Coverage 81.38% 81.45% +0.07%
==========================================
Files 1116 1120 +4
Lines 397960 401477 +3517
Branches 397960 401477 +3517
==========================================
+ Hits 323880 327043 +3163
- Misses 55120 55296 +176
- Partials 18960 19138 +178 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
approved these changes
Aug 27, 2026
kosiew
left a comment
kosiew
left a comment
Contributor
There was a problem hiding this comment.
Thanks for working on this. The fix looks good to me. I left one non-blocking suggestion to add coverage for the partitioned path as well.
| .values() | ||
| .iter() | ||
| .any(|array| array.null_count() > 0); | ||
| .any(|array| array.logical_null_count() > 0); |
Contributor
There was a problem hiding this 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.
Contributor
Author
There was a problem hiding this comment.
Sure! Thanks your suggestion
Contributor
Author
|
Sorry, I though auto-merge will be disabled if the test failed |
Contributor
|
Seems it causes CI fail but that is probably what @jayzhan211 is referring to abve |
alamb
pushed a commit
to tohuya6/datafusion
that referenced
this pull request
Aug 27, 2026
…ions handling (apache#24729) ## Which issue does this PR close? - resolved apache#24734 apache#24587 is auto merged with failed test, this fix the test. The issue is something from `feature = "force_hash_collisions"` <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes #. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. Please explain the problem you are trying to solve in terms of the user-visible behavior, rather than the implementation. For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the implementation. "COUNT(DISTINCT) returns wrong results when the column contains nulls" is the user-visible problem. --> ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR. --> ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please add the `api change` label. -->
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
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.
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() == 0butlogical_null_count() > 0), and two places in the hash join only looked at the physical count:NullEqualsNulljoin,keys_have_nullwas computed withnull_count(), so the pushed dynamic filter was not widened withkey IS NULLand pruned the probe-side NULL row that should have null-matched the build-side NULL.try_create_array_mapinferred "the key type is supported byArrayMap" 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 withUnsupported type for ArrayMap: Dictionary(Int32, Int32)instead of falling back to the regular hash map.What changes are included in this PR?
stream.rs: computekeys_have_nullwithlogical_null_count().exec.rs: gateArrayMap(perfect hash join) creation on the key type viais_perfect_hash_join_candidaterather than on the presence of bounds; also uselogical_null_count()for theNullEqualsNullcheck in the same function.should_collect_min_max_for_perfect_hash→is_perfect_hash_join_candidatewith 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 aNullEqualsNullinner join over dictionary keys with a dynamic filter consumer on the probe side and asserts both that the pushed filter contains theIS NULLdisjunct and that the NULL–NULL row is produced. Before this PR it fails with theArrayMaperror; with only theArrayMapgate it fails on the missingIS NULL. Existing hash join unit tests and thejoins,push_down_filter_parquet, andexplain_analyzesqllogictests 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.