Repository navigation
test: add an SQL reproducer for the canceled hash join partition hang - #27345
Conversation
The fix for apache#21625 has only a Rust unit test. No SQL test covers the plan shape that hung TPC-H Q18. This adds a sqllogictest that reproduces the hang deterministically. A partitioned inner join sits on top of a partitioned child join on the same key. The parent build side holds one key, so 3 of 4 parent partitions are empty and drop their child partitions before those report to the dynamic filter. An EXPLAIN assertion locks the plan shape the reproducer needs. Without the fix in apache#21666, the query hangs. With the fix, it returns 2 rows. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@kosiew could I have you take a look at this? |
There was a problem hiding this comment.
Thanks for adding SQL-level regression coverage for the canceled hash join partition hang. The test constructs the required nested partitioned join shape, verifies it through EXPLAIN, and checks that execution completes with the expected duplicate-preserving result.
I found no correctness or compatibility concerns, and the test passes locally. The inline suggestions are optional maintenance improvements, not acceptance requirements. I am approving the change.
| # The reproducer needs this exact shape: both joins Partitioned, `s` on the | ||
| # parent build side, and the child join directly on the parent probe side. | ||
| query TT | ||
| EXPLAIN SELECT s.k, ab.v |
There was a problem hiding this comment.
Consider setting datafusion.explain.physical_plan_only = true around this EXPLAIN and resetting it afterward. The physical plan already captures all cancellation prerequisites, while the logical-plan snapshot adds maintenance overhead from unrelated formatting changes.
| statement ok | ||
| set datafusion.execution.target_partitions = 4; | ||
|
|
||
| # Force both joins into Partitioned mode. |
There was a problem hiding this comment.
Consider explicitly setting datafusion.optimizer.enable_dynamic_filter_pushdown = true and resetting it during cleanup to document the coordinator prerequisite. This also enables join dynamic-filter pushdown, so a separate setting is unnecessary; both currently default to true, and the existing EXPLAIN already guards against missing filters.
Which issue does this PR close?
Rationale for this change
#21666 fixed a hang in partitioned hash joins with dynamic filters (first seen as TPC-H Q18 hanging). Its only test is a Rust unit test of the coordinator. No SQL-level test covers the plan shape that triggers the hang, so a planner or execution change could bring it back unnoticed.
What changes are included in this PR?
One new file,
join_dynamic_filter_canceled_partition.slt. It is a small, deterministic reproducer:Partitionedinner join sits on top of aPartitionedchild join on the same key, with no repartition between them.k = 1, so 3 of the 4 parent partitions have an empty build side. They complete without polling their probe side, so their child partitions are dropped before they report to the shared dynamic-filter coordinator.The hang does not depend on timing: the hash routing of
k = 1is fixed, and a child partition reports only when its parent polls it. AnEXPLAINassertion locks the plan shape the reproducer needs. The file comments link to the issue and describe the sequence of events.What is the testing strategy for this PR?
main.report_canceled_partitioncall inHashJoinStream::dropreplaced by a no-op), the test hangs. It never completed and was killed by a 500 s timeout.datafusion-cli, the same query hangs 5 of 5 times at887595660^and returns 2 rows 5 of 5 times at887595660.Note: a regression shows up as a hang, not a failed assertion, so CI would catch it by the job timeout.
Are there any user-facing changes?
No. Test-only.
🤖 Generated with Claude Code