Skip to content

[BugFix]: Swap join children, not left and right set #16487

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Mar 4, 2025
Merged

[BugFix]: Swap join children, not left and right set #16487

merged 2 commits into from
Mar 4, 2025
+30 −2

Conversation

Tmonster
Copy link
Contributor

@Tmonster Tmonster commented Mar 3, 2025

Only the left and right set were being swapped, but not the actual join children. I think this case is very rare, hence why it has not been caught in our current CI suite.

Fixes #16426
Fixes https://github.com/duckdblabs/duckdb-internal/issues/4323

@duckdb-draftbot duckdb-draftbot marked this pull request as draft March 3, 2025 14:20
@Tmonster Tmonster marked this pull request as ready for review March 3, 2025 14:21
@Tmonster
Copy link
Contributor Author

Tmonster commented Mar 4, 2025

@Mytherin I think the tpcds regression is a false alarm. I ran the plan_cost_runner on the queries as well and there was no difference detected, meaning the amount of intermediate tuples produced at every join was the same. Looking at the plans I also don't see any difference.

Local benchmarking doesn't reveal much of a difference

With Fix

./build/release/benchmark/benchmark_runner benchmark/tpcds/sf1/q04.benchmark
name	run	timing
benchmark/tpcds/sf1/q04.benchmark	1	0.106031
benchmark/tpcds/sf1/q04.benchmark	2	0.080135
benchmark/tpcds/sf1/q04.benchmark	3	0.098354
benchmark/tpcds/sf1/q04.benchmark	4	0.080948
benchmark/tpcds/sf1/q04.benchmark	5	0.086832

Without Fix

name	run	timing
benchmark/tpcds/sf1/q04.benchmark	1	0.080774
benchmark/tpcds/sf1/q04.benchmark	2	0.086536
benchmark/tpcds/sf1/q04.benchmark	3	0.083359
benchmark/tpcds/sf1/q04.benchmark	4	0.087681
benchmark/tpcds/sf1/q04.benchmark	5	0.085415

@Mytherin Mytherin merged commit fba0ffe into duckdb:v1.2-histrionicus Mar 4, 2025
49 of 50 checks passed
@Mytherin
Copy link
Collaborator

Mytherin commented Mar 4, 2025

Thanks! LGTM

krlmlr added a commit to duckdb/duckdb-r that referenced this pull request Mar 7, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@fba0ffe
9acb564 
[BugFix]: Swap join children, not left and right set (duckdb/duckdb#16487)
bump sqlsmith extension tag (duckdb/duckdb#16488)
Bump `postgres_scanner` and `fts` extensions (duckdb/duckdb#16492)
[tests] Multiple FORMAT in copy, only last one matters (duckdb/duckdb#16493)
[Python Dev] `pyproject.toml` should not use `oldest-supported-numpy` anymore (duckdb/duckdb#16486)
Fixed reading piped JSON (duckdb/duckdb#16480)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Tmonster @Mytherin