Repository navigation
fix(substrait): roundtrip conditionless joins - #23469
Conversation
0f01204 to
35f7501
Compare
35f7501 to
be97231
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23469 +/- ##
==========================================
- Coverage 82.72% 82.72% -0.01%
==========================================
Files 1147 1147
Lines 448992 449193 +201
Branches 448992 449193 +201
==========================================
+ Hits 371439 371598 +159
- Misses 54918 54941 +23
- Partials 22635 22654 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
394bced to
698093a
Compare
698093a to
88909c8
Compare
|
@neilconway, would you have some time to review this one? it fixes scalar-subquery projection round-trips when the rewritten join has no condition. the producer now emits a valid |
|
@bvolpato Sure! I'm traveling for a few days but I'm happy to take a look next week. |
88909c8 to
a2d9128
Compare
|
@neilconway, thanks again for offering to take a look. I pushed a follow-up fix for conditionless joins with overlapping input names, and CI is green on the latest head. Could you review when you have a chance? |
neilconway
left a comment
There was a problem hiding this comment.
Thanks @bvolpato ! Sorry for the delay on this.
Overall this looks good, but as far as I can tell, it isn't specific to scalar subqueries? e.g., the same issue arises for queries containing x LEFT JOIN y ON true or WHERE EXISTS (select 1 from x). The problem is really that condition-less joins didn't roundtrip correctly. If you agree, can you update the PR description accordingly?
I think it would also make sense to test some of these cases via SLT -- since if/when we remove the old-style approach to evaluating scalar subqueries, the SLT we're adding for that case will be removed then.
Only combine input schemas when serializing an existing join predicate. Use the valid output schema for the synthetic true literal, allowing conditionless semi, anti, and mark joins with overlapping input names. Cover both qualified and unqualified inputs for all six join variants.
220bc28 to
db73510
Compare
|
thanks for the review, Neil. agreed, the fix is about conditionless joins more broadly. I updated the title and description accordingly. I added SQL tests for outer joins and uncorrelated EXISTS / NOT EXISTS, including empty inputs. They're also run in optimized Substrait round-trip mode in CI, so they exercise the join shapes created by the optimizer. Removing the producer fix makes seven of the new cases fail with |
|
Thanks @bvolpato ! |
Which issue does this PR close?
Rationale for this change
Queries with conditionless joins can fail when their optimized logical plans roundtrip through Substrait, with
Plan("join condition should not be empty"). This affectsLEFT JOIN ... ON trueand uncorrelatedWHERE EXISTS, as well as scalar-subquery projections when the legacy scalar-subquery rewrite is enabled.The optimizer can remove a constant
truejoin filter. The producer then omittedJoinRel.expression, which Substrait requires.What changes are included in this PR?
CrossReland other conditionless joins with a literaltruecondition.EXISTS/NOT EXISTS, including empty inputs.--substrait-optimizetest mode and run the new fixture through it in CI. This exercises the optimized plans that trigger the bug.What is the testing strategy for this PR?
joins_conditionless.sltchecks query results through ordinary SQL execution and optimized Substrait roundtripping. Existing producer and DataFrame tests cover conditionless inner joins, scalar-subquery projections, and overlapping input names.Local validation passed: formatting, Clippy with all targets and features, the extended workspace suite, and
./dev/rust_lint.sh. The extended suite ran in an isolated PID namespace so the RSS tests could sample a valid baseline on this host. Removing the producer fix makes seven of the new optimized roundtrip cases fail withjoin condition should not be empty; all pass with the fix restored.Are there any user-facing changes?
Queries containing conditionless joins can roundtrip through Substrait successfully. No breaking production API changes.