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

coral-hive: Added support for WITH clauses #125

Merged
merged 1 commit into from
Aug 19, 2021

Conversation

uzshao
Copy link
Contributor

@uzshao uzshao commented Aug 19, 2021

No description provided.

@uzshao
Copy link
Contributor Author

uzshao commented Aug 19, 2021

@wmoustafa @funcheetah @antumbde This is a follow-up diff to support WITH clauses in Hive. Please take a look when you get a chance.

Copy link
Collaborator

@ljfgem ljfgem left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @uzshao
Integration test succeeded.

@@ -62,6 +62,40 @@ public void testBasic() {
verifyRel(rel, expected);
}

@Test
public void testBasic2() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we share the common code between testBasic and testBasic2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just fixed it by extracting the common code into a separate function.

@wmoustafa
Copy link
Contributor

@findepi @losipiuk FYI.

@uzshao
Copy link
Contributor Author

uzshao commented Aug 19, 2021

@wmoustafa Can you take a look again? I extracted the common code, and also added one more test case on with.


@Test
public void testWithNested() {
// Test if the code can handle the use of the second alias from the WithList
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: the comment is the same as testWith2, should also be changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! Just pushed a fix to the comment.

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