Skip to content

fix: do not recompute hash join exec properties if not required - #20900

Merged
alamb merged 1 commit into
apache:mainfrom
askalt:askalt/fix-hash-join-with-new-children-perf
Mar 12, 2026
Merged

fix: do not recompute hash join exec properties if not required#20900
alamb merged 1 commit into
apache:mainfrom
askalt:askalt/fix-hash-join-with-new-children-perf

Conversation

@askalt

@askaltaskalt commented Mar 12, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

The check on same properties was accidentally deleted which led to reset_plan_states benchmark performance drop: from ~2us to ~3ms. This patch fixes it.

@github-actionsgithub-actionsBot added the physical-plan Changes to the physical-plan crate label Mar 12, 2026
@askalt
askaltforce-pushed the askalt/fix-hash-join-with-new-children-perf branch from c4ba6d9 to e0cb11aCompareMarch 12, 2026 10:42
@askaltaskalt mentioned this pull request Mar 12, 2026
Closed
26 tasks
@alamb

alamb commented Mar 12, 2026

Copy link
Copy Markdown
Contributor

run benchmark reset_plan_states

@alamb-ghbot

alamb-ghbot commented Mar 12, 2026

Copy link
Copy Markdown

🤖 ./gh_compare_branch_bench.shcompare_branch_bench.sh Running
Linux aal-dev 6.14.0-1018-gcp #19~24.04.1-Ubuntu SMP Wed Sep 24 23:23:09 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing askalt/fix-hash-join-with-new-children-perf (e0cb11a) to 8d9b080diff
BENCH_NAME=reset_plan_states
BENCH_COMMAND=cargo bench --features=parquet --bench reset_plan_states
BENCH_FILTER=
BENCH_BRANCH_NAME=askalt_fix-hash-join-with-new-children-perf
Results will be posted here when complete

alamb
alamb approved these changes Mar 12, 2026
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me -- thank you @askalt

Comment threaddatafusion/physical-plan/src/execution_plan.rs
/// The size of `children` must be equal to the size of `ExecutionPlan::children()`.
pub fn has_same_children_properties(
plan: &Arc<impl ExecutionPlan>,
plan: &impl ExecutionPlan,

@alambalambMar 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is needed because the builder doesn't have an Arc, right?

@askaltaskaltMar 12, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is so. It looks ok to change this argument type as this API was not released yet.

@alambalambMar 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

@alamb-ghbot

alamb-ghbot commented Mar 12, 2026

Copy link
Copy Markdown

🤖: Benchmark completed

Details

group askalt_fix-hash-join-with-new-children-perf main
----- ------------------------------------------- ----
query1 1.00 2.6±0.01µs ? ?/sec 1.00 2.6±0.02µs ? ?/sec
query2 1.00 3.3±0.01µs ? ?/sec 1469.86 4.8±0.01ms ? ?/sec
query3 1.01 987.7±13.93ns ? ?/sec 1.00 980.8±3.79ns ? ?/sec

@alamb

alamb commented Mar 12, 2026

Copy link
Copy Markdown
Contributor

query2 1.00 3.3±0.01µs ? ?/sec 1469.86 4.8±0.01ms ? ?/sec

🚀

@alamb
alamb added this pull request to the merge queueMar 12, 2026
Merged via the queue into apache:main with commit 4b022c0Mar 12, 2026
34 checks passed
alamb pushed a commit to alamb/datafusion that referenced this pull request Mar 12, 2026
@askalt@alamb
fix: do not recompute hash join exec properties if not required (apac…
27b7ff9
…he#20900)
## Rationale for this change
The check on same properties was accidentally deleted which led to
`reset_plan_states` benchmark performance drop: from ~2us to ~3ms. This
patch fixes it.
@alambalamb mentioned this pull request Mar 12, 2026
Merged
alamb added a commit that referenced this pull request Mar 12, 2026
@alamb@askalt
[branch-53] fix: do not recompute hash join exec properties if not re…
e88a5fa
…quired (#20900) (#20903)
- Part of #19692
- Backports #20900 from @askalt
to the branch-53 line
This PR:
- Backports #20900 to branch-53
Co-authored-by: Albert Skalt <133099191+askalt@users.noreply.github.com>
de-bgunter pushed a commit to de-bgunter/datafusion that referenced this pull request Mar 24, 2026
@askalt@de-bgunter
fix: do not recompute hash join exec properties if not required (apac…
f11895f
…he#20900)
## Rationale for this change
The check on same properties was accidentally deleted which led to
`reset_plan_states` benchmark performance drop: from ~2us to ~3ms. This
patch fixes it.
This was referenced Apr 4, 2026
Closed
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb approved these changes

Assignees

No one assigned

Labels

physical-planChanges to the physical-plan crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@askalt@alamb@alamb-ghbot