[VL] Fix broadcast hash table reuse for reused exchanges#12264
[VL] Fix broadcast hash table reuse for reused exchanges#12264wecharyu wants to merge 3 commits into
Conversation
|
Run Gluten Clickhouse CI on x86 |
| } | ||
| } | ||
|
|
||
| override def doCanonicalizeForBroadcastMode(mode: BroadcastMode): BroadcastMode = { |
There was a problem hiding this comment.
Is this part still needed when buildHashTableOncePerExecutor is disabled?
There was a problem hiding this comment.
doCanonicalizeForBroadcastMode() is not invoked anywhere, broadcast canonicalization goes through ColumnarBroadcastExchangeExec.doCanonicalize().
There was a problem hiding this comment.
This code is actually useful — when buildHashTableOncePerExecutor is disabled, it provides more opportunities to reuse broadcast exchanges. Moreover, I now think that even when buildHashTableOncePerExecutor is enabled, the comment in doCanonicalizeForBroadcastMode still holds true: we still broadcast byte arrays and build HashRelation at the executor side.
@JkSelf Can you explain why this was removed? This allows us to reuse broadcast exchanges for different build keys with the same data.
We should either restore the code before ColumnarBroadcastExchangeExec.doCanonicalize, or at least follow the original logic when buildHashTableOncePerExecutor is disabled.
There was a problem hiding this comment.
|
Run Gluten Clickhouse CI on x86 |
wecharyu
force-pushed
the
fix_shared_bhj_table
branch
from
075dc89 to
7552606
Compare
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
wecharyu
force-pushed
the
fix_shared_bhj_table
branch
from
d7d69eb to
d88b181
Compare
|
Run Gluten Clickhouse CI on x86 |
|
@wecharyu Thanks for your work. For ColumnarBroadcastExchangeExec, reuse is already decided by doCanonicalize(). It now directly uses mode.canonicalized, and that canonicalized BroadcastMode already captures the key BHJ semantics such as bound build keys and null-aware behavior. So we no longer need this handling for the unique broadcastId. |
|
@JkSelf Thanks for the prompt response. And since current Gluten is still broadcast the raw build plan data bytes instead of hash table,I agree with @liujiayi771 that I think now we can make following improvements:
|
Gluten's implementation is the same as vanilla Spark's. Just out of curiosity, can Spark pass this failed test? |
Vanilla Spark does share the same hash relation, but it does not drop duplicates, so the two types of joins will not generate data issue. But Gluten native build hash table will drop duplicates for the |
wecharyu
force-pushed
the
fix_shared_bhj_table
branch
from
d88b181 to
8e7e9b5
Compare
|
Run Gluten Clickhouse CI on x86 |
…erExecutor is disabled
wecharyu
force-pushed
the
fix_shared_bhj_table
branch
from
8e7e9b5 to
da8f1f6
Compare
|
Run Gluten Clickhouse CI on x86 |
|
@JkSelf @liujiayi771 Pls take a look again when you have time. Thanks! |
| case class BroadcastHashTable( | ||
| pointer: Long, | ||
| relation: BuildSideRelation, | ||
| droppedDuplicates: Boolean) |
There was a problem hiding this comment.
Do we need to consider ExistenceJoin and null-aware anti join value? Could you help to add tests to cover? Thanks.
3 participants
What changes are proposed in this pull request?
Cache hash table data by
droppedDuplicatesflag in build side relation because the reused relation could generate different hash table data.How was this patch tested?
Add UT.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Codex GPT-5.5