perf: Eliminate a join a foreign key makes redundant - #24793
Draft
Dandandan wants to merge 1 commit into
Draft
Conversation
When a fact table joins a dimension on a foreign key, uses none of the dimension's columns, and does not filter it, the join only checks that a row that is guaranteed to exist does exist. `eliminate_join` already turns that into a semi join; with the foreign key declared it can drop the join entirely. DataFusion had no way to declare one. `Constraint` gains a `ForeignKey` variant, the SQL planner accepts `FOREIGN KEY (..) REFERENCES t(..)` and the inline `REFERENCES` form instead of rejecting them, and both round-trip through proto. The join is replaced by the left input under a `col IS NOT NULL` filter rather than dropped outright. A foreign key column may be nullable, and a semi join drops rows whose key is NULL, so the filter preserves that. It also covers an outer join further down having padded the column with NULLs. The rewrite only applies when the referenced side is an unfiltered scan of the referenced table: a foreign key promises the value exists in the table, not that it survives a predicate. `dfbench` declares the TPC-DS foreign keys, which its comment previously noted as unsupported. On TPC-DS SF1 that removes 6 joins from q64, taking it from 347 ms to 239 ms (1.45x), and q18 from 92 ms to 76 ms (1.22x), for -1.2% on the suite. All 99 queries return unchanged results. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgqwvctZdvR1ZCz2hbkEJC
github-actions
Bot
added
sql
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24793 +/- ##
==========================================
- Coverage 81.52% 81.50% -0.03%
==========================================
Files 1123 1123
Lines 405970 406320 +350
Branches 405970 406320 +350
==========================================
+ Hits 330978 331162 +184
- Misses 55627 55781 +154
- Partials 19365 19377 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
run benchmark tpcds |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing feat/fk-join-elimination (d043a7e) to 61bf6b9 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing feat/fk-join-elimination (d043a7e) to 61bf6b9 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
Contributor
Author
|
run benchmark tpcds |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing feat/fk-join-elimination (d043a7e) to 61bf6b9 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing feat/fk-join-elimination (d043a7e) to 61bf6b9 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
A fact table often joins a dimension purely to check that a row exists:
No column of
promotionis used and nothing filters it.eliminate_joinalready turns this into a semi join, because
p_promo_skis a primary key. Butif a foreign key says every
ss_promo_skexists inpromotion, the check isguaranteed to succeed and the join can be dropped entirely.
DataFusion had no way to say that.
Constrainthad onlyPrimaryKeyandUnique, and the SQL planner rejected foreign keys outright with "Foreign keyconstraints are not currently supported".
What changes are included in this PR?
Constraint::ForeignKey { columns, referenced_table, referenced_columns },taken on trust like the existing constraints, round-tripping through proto.
FOREIGN KEY (..) REFERENCES t(..)and the inlinecol INT REFERENCES t(c)form.eliminate_joindrops a join the foreign key makes redundant.dfbenchdeclares the TPC-DS foreign keys. Its comment previously read"TPC-DS also defines foreign keys, but those are currently unsupported".
Two details worth review:
The join is replaced by its left input under a
col IS NOT NULLfilter, notdropped outright. A foreign key column may be nullable, and the semi join it
replaces does drop rows whose key is NULL, so the filter preserves that. It
also covers the case where an outer join further down padded the column.
The rewrite only fires when the referenced side is an unfiltered scan of the
referenced table. A foreign key promises the value exists in the table, not
that it survives a predicate, so a filtered dimension keeps its join.
Benchmarks
TPC-DS SF1, primary keys declared in both runs, median of 3, plus the movers
re-checked at 9 iterations:
q64 loses 6 joins:
promotiontwice andincome_bandfour times. q23 and q24looked like regressions at 3 iterations but their plans are unchanged, and at 9
iterations they are flat.
TPC-H is unaffected: all of its dimension joins carry filters.
Are these changes tested?
Yes.
functional_dependencies.sltcovers the rewrite, the results it produces,and the two cases that must keep the join: a predicate on the referenced side,
and a query that uses one of its columns. Three cases in
group_by.sltthatasserted foreign keys were rejected now assert they are accepted.
All 99 TPC-DS queries were checked to return identical results with and without
the foreign keys declared. The full sqllogictest suite (504 files) and the
workspace test suite pass.
Are there any user-facing changes?
FOREIGN KEYinCREATE TABLEis accepted instead of erroring, and joins itmakes redundant are removed.
Constraintgains a variant, so exhaustive matcheson it need a new arm; this is an API change.