Skip to content
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

feat: Add support for unnest with lateral join #309

Merged
merged 46 commits into from
Aug 12, 2022

Conversation

gaurav274
Copy link
Member

No description provided.

@gaurav274 gaurav274 marked this pull request as ready for review August 10, 2022 09:29
@gaurav274 gaurav274 requested a review from xzdandy August 10, 2022 23:08
alias_name = self.visit(ctx.uid())
column_list = []
if ctx.uidList():
column_list = self.visit(ctx.uidList())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it becomes harder over time to figure out which visit function it actually calls. Any idea to improve the readability?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually quite intuitive, thanks to antlr.
self.visit(ctx.uidList()) -> visitUidList

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it is somehow in the _insert_statements.py. I missed it.


self.assertEqual(unnest_batch, expected)

def test_should_raise_error_with_missing_alias_in_lateral_join(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So JOIN LATERAL much have AS. However, it seems from https://github.com/georgia-tech-db/eva/blob/3c16181b140bc3b8cd26e99212d73f14a479ba30/eva/binder/statement_binder.py#L281 , when the node does not have alias, it will use the default node name and column name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have added the code to handle it. But later felt that avoiding it, for now, is better. We can later remove the check from the parser and test it more.

Copy link
Collaborator

@xzdandy xzdandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jarulraj jarulraj merged commit 4a92406 into georgia-tech-db:master Aug 12, 2022
@gaurav274
Copy link
Member Author

#144

@jarulraj jarulraj mentioned this pull request Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants