Repository navigation
Make Join predicate condition necessary for Inner Joins - #4746
Closed
shivanshrustagi wants to merge 7 commits into
Closed
shivanshrustagi wants to merge 7 commits into
shivanshrustagi wants to merge 7 commits into
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.
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.
bug: Currently if the user issues an inner join without `ON` or `USING` join predicate the query would fail internally due to null pointer exception fix: Made the grammar in-line with the behaviour where any `INNER JOIN` (or just `JOIN`) would be rejected by Grammar as a SYNTAX error and handles the exception gracefully. fixes FoundationDB#4722
📊 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. |
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
1 participant
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.
bug: Currently if the user issues an inner join without
ONorUSINGjoin predicate the query would fail internally due to null pointer
exception
fix: Made the grammar in-line with the behaviour where any
INNER JOIN(or just
JOIN) would be rejected by Grammar as a SYNTAX error andhandles the exception gracefully.
fixes #4722