Repository navigation
Add Support for CROSS JOIN - #4721
Conversation
CROSS JOIN now has its own grammar rule and visitor method. Previously it was parsed as an inner join with an optional ON / USING. a CROSS JOIN b now means the same as FROM a, b: the Cartesian product, optionally filtered by WHERE. Changes - Grammar: CROSS is removed from #innerJoin, and a new #crossJoin alternative is added: CROSS JOIN tableSourceItem, with no ON or USING. - Visitor: QueryVisitor.visitCrossJoin adds the right-hand table to the current plan fragment and no join predicate. visitSimpleTable later merges all tables into one flat SelectExpression, as it already does for inner joins. - Planner: unchanged. The planner receives the same SelectExpression as for the comma syntax, and explain assertions confirm the plans are identical. Testing The new cross-join-tests.yamsql covers: - the Cartesian product, compared with the comma syntax - column resolution, including ambiguous and undefined columns - WHERE as the join predicate, compared with JOIN … ON - chains mixing CROSS with inner, USING, comma and LEFT joins - self-joins, derived tables and correlated EXISTS / NOT EXISTS - empty inputs - syntax that the grammar rejects. Known issue (FoundationDB#4170) When an equality join is planned as an index or primary-key lookup, a NULL key matches other NULL keys. The cause is ScanComparisons.toTupleRange(). This isn't specific to cross joins: JOIN … ON and comma joins behave the same way. The affected tests are marked TODO Issue FoundationDB#4170.
shivanshrustagi
added
the
Run mixed-mode
hatyo
left a comment
hatyo
left a comment
There was a problem hiding this comment.
The PR is good, though I think we should mark it as a breaking change. See my comment.
| # Syntax Rejected by Grammar | ||
| - | ||
| # CROSS JOIN does not accept an ON join predicate | ||
| - query: SELECT fname, lname, name FROM emp CROSS JOIN dept ON emp.dept_id = dept.id |
There was a problem hiding this comment.
this syntax was accepted before your change, but now it is correctly rejected, this is fine, but we might have customer using that syntax, so their queries will break if they attempt to consume a newer record-layer version with this PR.
This means we should probably tag this PR as breaking change.
There was a problem hiding this comment.
I think this PR is backward and forward compatible when it comes to continuation handling though, so marking it as breaking change is a bit heavy handed ...
There was a problem hiding this comment.
Removing the label.
shivanshrustagi
added
the
breaking change
alecgrieser
left a comment
alecgrieser
left a comment
There was a problem hiding this comment.
LGTM! I like the breadth of the new test cases here. Teamscale is flagging two test gaps, but once those are covered (probably by adding a unit test to DeletegatingVisitorTest), this should be good to go.
| @Nullable | ||
| @Override | ||
| public Object visitCrossJoin(@Nonnull RelationalParser.CrossJoinContext ctx) { | ||
| return getDelegate().visitCrossJoin(ctx); |
There was a problem hiding this comment.
Teamscale is marking this as a test gap. The exact flow here is a little subtle, but I believe this is because we only ever try to visit the crossJoin nodes from within a QueryVisitor, and so the QueryVisitor's visitCrossJoin is the only one that gets called.
I think that's the right approach for this code, so I think we just need to add some tests to make sure that we do delegate appropriately in case we change how that works in the future. We have a test file (DelegatingVisitorTest) that we use for adding tests of that. The tests in their also allow for testing the BaseVisitor.
There was a problem hiding this comment.
Thanks Alec, noted. Will update the PR with the Unit test.
shivanshrustagi
removed
the
breaking change
Updated the documentation to include CROSS JOIN examples and more information on the usage and limtiations of CROSS JOIN. Added a few more additional tests to compare the equivalence of CROSS JOIN ... WHERE with INNER JOIN ... ON in certain conditions
During the logical plan creation, BaseVisitor's visitCrossJoin should not be reached since the BaseVisitor hands the responsibility to the QueryVisitor. The test verifies that the BaseVisitor's visitCrossJoin should not be invoked and the responsibility remains with QueryVisitor. If the expectation or code handling changes, the test should also change to reflect this
Teamscale flagged DelegatingVisitor.visitCrossJoin() and BaseVisitor.visitCrossJoin() as test gaps: during plan generation the joinPart nodes are visited from within the QueryVisitor, so only QueryVisitor.visitCrossJoin() is ever called. visitCrossJoinTest checks that DelegatingVisitor.visitCrossJoin() forwards the call to its delegate. The override calls super, so BaseVisitor.visitCrossJoin() is called as well. visitAtomTableItem is stubbed because only a joinPart is parsed due to incomplete query and no plan fragment exists.
37bee2e to
7f08f3a
Compare
📊 Metrics Diff Analysis ReportSummary
ℹ️ About this analysisThis automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:
The last category in particular may indicate planner regressions that should be investigated. New QueriesCount of new queries by file:
|
3 participants
CROSS JOINnow has its own grammar rule and visitor method. Previously it was parsed as an inner join with an optionalON/USING.a CROSS JOIN bnow means the same asFROM a, b: the Cartesian product, optionally filtered byWHERE.Changes:
CROSSis removed from#innerJoin, and a new#crossJoinalternative is added:CROSS JOIN tableSourceItem, with noONorUSING.QueryVisitor.visitCrossJoinadds the right-hand table to the current plan fragment and no join predicate.visitSimpleTablelater merges all tables into one flatSelectExpression, as it already does for inner joins.SelectExpressionas for the comma syntax, andexplainassertions confirm the plans are identical.Testing
The new
cross-join-tests.yamsqlcovers:WHEREas the join predicate, compared withJOIN … ONCROSSwith inner,USING, comma andLEFTjoinsEXISTS/NOT EXISTSKnown issue (#4170)
When an equality join is planned as an index or primary-key lookup, a NULL key matches other NULL keys. The cause is
ScanComparisons.toTupleRange(). This isn't specific to cross joins:JOIN … ONand comma joins behave the same way. The affected tests are markedTODO Issue #4170.