-
Notifications
You must be signed in to change notification settings - Fork 119
Support of INNER JOIN syntax
#3799
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.
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
Support of INNER JOIN syntax
#3799
Conversation
hatyo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, and congrats on your first PR. Few minor comments and we can bring it in.
| return Assert.castUnchecked(ctx.tableSourceItem().accept(this), LogicalOperator.class); | ||
| public Expression visitInnerJoin(@Nonnull RelationalParser.InnerJoinContext ctx) { | ||
| getDelegate().getCurrentPlanFragment().addOperator(Assert.castUnchecked(ctx.tableSourceItem().accept(this), LogicalOperator.class)); | ||
| return Assert.castUnchecked(ctx.expression().accept(this), Expression.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes that the user always provides an expression, in other words, they always use the syntax on expression, however if they mistakenly use the USING syntax instead, which is permissible as per the grammar rule:
(INNER | CROSS)? JOIN tableSourceItem
(
ON expression
| USING '(' uidList ')' // <<< on OR using
)? #innerJoin
then we get an NPE here (because ctx.expression() is null). It would be better if we catch this case and throw a proper SQLException with ErrorCode.UNSUPPORTED_QUERY.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added exception using is not yet supported for inner join and test case.
But it looks like a good time to implement using syntax properly and close the inner join case.
| Assert.thatUnchecked(ctx.joinPart().isEmpty(), "explicit join types are not supported"); | ||
| return Assert.castUnchecked(ctx.tableSourceItem().accept(this), LogicalOperator.class); | ||
| public Expression visitInnerJoin(@Nonnull RelationalParser.InnerJoinContext ctx) { | ||
| getDelegate().getCurrentPlanFragment().addOperator(Assert.castUnchecked(ctx.tableSourceItem().accept(this), LogicalOperator.class)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to handle all the interactions with current plan fragment here? instead of visitableTableSourceBase?
i.e. when calling vistInnerJoin we add both the tableSourceItem and expression to the current plan fragment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense!
Addressed
…ayer into inner_join
hatyo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Support of
INNER JOINsyntaxThis PR introduces the standard syntax for
INNER JOINoperation.Simple case
The requests like
SELECT t1.a, t2.b FROM t1 INNER JOIN t2 ON t1.id = t2.idare executed the same as
SELECT t1.a, t2.b FROM t1, t2 WHERE t1.id = t2.idBase case
SELECT select-expr FROM t1 INNER JOIN t2 ON join-expr1 INNER JOIN t3 ON join-expr2 INNER JOIN t4 ON join-expr3 WHERE where-exprworks the same as
SELECT select-expr FROM t1, t2, t3, t4 WHERE where-expr AND join-expr1 AND join-expr2 AND join-expr3