fix: `HashJoin` panic with String dictionary keys (don't flatten keys) by alamb · Pull Request #20505 · apache/datafusion

alamb · 2026-02-23T19:41:36Z

Which issue does this PR close?

Fixes INNER JOIN with dictionary keys fails when run on parquet with pushdown_filters = true. #20696
Follow on to fix: HashJoin panic with dictionary-encoded columns in multi-key joins #20441

Rationale for this change

#20441 (review) fixes the special case DictionaryArray handling in Joins. However, I don't think we need to special case DictionaryArrays at all

What changes are included in this PR?

Remove the special case dictionary handling

Are these changes tested?

Yes by CI

Are there any user-facing changes?

No (though maybe some queries get faster)

adriangb · 2026-02-23T20:59:41Z

I think the question is going to be if it still prunes all the same (including stats). I'd guess it should but we also probably added it there for a reason. I'd guess it was me and I did not add a failing test, which is not good.

alamb · 2026-02-24T12:13:05Z

I think the question is going to be if it still prunes all the same (including stats). I'd guess it should but we also probably added it there for a reason. I'd guess it was me and I did not add a failing test, which is not good.

Makes sense to me -- what type of pruning do you have in mind? I can write a test.

Like it would be "join a dictionary column and show that the predicate that got pushed down could prune"?

Or is the concern that when reading a string column from a parquet file as a dictionary it won't be able to prune 🤔

…builder

alamb · 2026-03-04T15:51:06Z

It actually turns out that we hit a bug in our testing at InfluxDB that this PR fixes

INNER JOIN with dictionary keys fails when run on parquet with pushdown_filters = true. #20696

SO it isn't just a cleanup, it is actually a bug fix.

I have added a reproducer to this PR

alamb · 2026-03-04T15:52:13Z

datafusion/sqllogictest/test_files/parquet_filter_pushdown.slt

+statement ok
+set datafusion.execution.parquet.pushdown_filters = true;
+
+query RRR


this test fails with

Parquet error: External: Compute error: Error evaluating filter predicate: ArrowError(InvalidArgumentError("Can't compare arrays of different types"), Some(""))

Prior to this fix

alamb · 2026-03-04T15:58:14Z

datafusion/physical-plan/src/joins/hash_join/inlist_builder.rs

 pub(super) fn build_struct_inlist_values(
    join_key_arrays: &[ArrayRef],
 ) -> Result<Option<ArrayRef>> {
-    // Flatten any dictionary-encoded arrays


I am quite pleased that the "fix" is to delete code

The mark of a great engineer 😉

adriangb

This fixes a bug and I can't immediately think of any bugs this would cause / why this was needed in the first place, and there are no failing tests. Let's move forward with this and if we find a future bug caused by this change we can come back and think of a better way to fix it than the current code which is buggy.

alamb · 2026-03-04T15:59:45Z

This fixes a bug and I can't immediately think of any bugs this would cause / why this was needed in the first place, and there are no failing tests. Let's move forward with this and if we find a future bug caused by this change we can come back and think of a better way to fix it than the current code which is buggy.

Thanks @adriangb -- I will also spend some time trying to research when it came in and why

I think the question is going to be if it still prunes all the same (including stats). I'd guess it should but we also probably added it there for a reason. I'd guess it was me and I did not add a failing test, which is not good.

adriangb · 2026-03-04T16:07:33Z

Thanks @adriangb -- I will also spend some time trying to research when it came in and why

You should check with the author of #18393 that introduced that code.

I think maybe I just added that under the premise that "what's the point of having a dictionary array for InList if we are building a HashMap in front of it anyway?"

alamb · 2026-03-04T20:16:30Z

You should check with the author of #18393 that introduced that code.

😆

Indeed it was added in c0e8bb5 and as you say there is no test coverage / failure for it.

I think maybe I just added that under the premise that "what's the point of having a dictionary array for InList if we are building a HashMap in front of it anyway?"

Yeah, I also asked Codex about the PR and it didn't seem to find any problems

alamb · 2026-03-04T20:16:50Z

So I'll merge this one and then make a backport to branch-53

apache#20505) ## Which issue does this PR close? - Fixes apache#20696 - Follow on to apache#20441 ## Rationale for this change apache#20441 (review) fixes the special case DictionaryArray handling in Joins. However, I don't think we need to special case DictionaryArrays at all ## What changes are included in this PR? 1. Remove the special case dictionary handling ## Are these changes tested? Yes by CI ## Are there any user-facing changes? No (though maybe some queries get faster)

apache#20505) - Fixes apache#20696 - Follow on to apache#20441 apache#20441 (review) fixes the special case DictionaryArray handling in Joins. However, I don't think we need to special case DictionaryArrays at all 1. Remove the special case dictionary handling Yes by CI No (though maybe some queries get faster)

alamb · 2026-03-04T21:03:02Z

Backport PR

[branch-52] fix: HashJoin panic with String dictionary keys (don't flatten keys) (#20505) #20708

github-actions bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Feb 23, 2026

alamb mentioned this pull request Feb 23, 2026

fix: HashJoin panic with dictionary-encoded columns in multi-key joins #20441

Merged

Avoid flattening dictionaries in Join InLists

5d17c18

alamb force-pushed the alamb/cleanup_inlist_builder branch from 42fa12b to 5d17c18 Compare February 24, 2026 12:11

github-actions bot removed the sqllogictest SQL Logic Tests (.slt) label Feb 24, 2026

alamb marked this pull request as ready for review February 24, 2026 13:49

erratic-pattern mentioned this pull request Mar 3, 2026

Patched DF 52.1.0 (revision b) influxdata/arrow-datafusion#91

Open

alamb added 3 commits March 4, 2026 10:47

Add regression tests

2e3412b

Merge remote-tracking branch 'apache/main' into alamb/cleanup_inlist_…

d8e20cb

…builder

Update test after fix

3b25b36

alamb changed the title ~~Avoid flattening dictionaries in Join InLists~~ fix: HashJoin panic with String dictionary keys Mar 4, 2026

alamb commented Mar 4, 2026

View reviewed changes

alamb changed the title ~~fix: HashJoin panic with String dictionary keys~~ fix: HashJoin panic with String dictionary keys (don't flatten keys) Mar 4, 2026

alamb added the bug Something isn't working label Mar 4, 2026

alamb commented Mar 4, 2026

View reviewed changes

adriangb approved these changes Mar 4, 2026

View reviewed changes

alamb added this pull request to the merge queue Mar 4, 2026

Merged via the queue into apache:main with commit d025869 Mar 4, 2026
34 of 36 checks passed

alamb deleted the alamb/cleanup_inlist_builder branch March 4, 2026 20:55

alamb mentioned this pull request Mar 4, 2026

[branch-52] fix: HashJoin panic with String dictionary keys (don't flatten keys) (#20505) #20708

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: `HashJoin` panic with String dictionary keys (don't flatten keys)#20505

fix: `HashJoin` panic with String dictionary keys (don't flatten keys)#20505
alamb merged 4 commits intoapache:mainfrom
alamb:alamb/cleanup_inlist_builder

alamb commented Feb 23, 2026 •

edited

Loading

Uh oh!

adriangb commented Feb 23, 2026

Uh oh!

alamb commented Feb 24, 2026

Uh oh!

alamb commented Mar 4, 2026

Uh oh!

alamb Mar 4, 2026

Uh oh!

alamb Mar 4, 2026

Uh oh!

adriangb Mar 4, 2026

Uh oh!

adriangb left a comment

Uh oh!

alamb commented Mar 4, 2026

Uh oh!

adriangb commented Mar 4, 2026 •

edited

Loading

Uh oh!

alamb commented Mar 4, 2026

Uh oh!

alamb commented Mar 4, 2026

Uh oh!

Uh oh!

alamb commented Mar 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

alamb commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

adriangb commented Feb 23, 2026

Uh oh!

alamb commented Feb 24, 2026

Uh oh!

alamb commented Mar 4, 2026

Uh oh!

alamb Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

adriangb Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

adriangb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Mar 4, 2026

Uh oh!

adriangb commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Mar 4, 2026

Uh oh!

alamb commented Mar 4, 2026

Uh oh!

Uh oh!

alamb commented Mar 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alamb commented Feb 23, 2026 •

edited

Loading

adriangb commented Mar 4, 2026 •

edited

Loading