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

refactor: implement ExpressionVisitor in SqlToJava #3130

Merged

Conversation

rodesai
Copy link
Contributor

@rodesai rodesai commented Jul 25, 2019

SqlToJavaVisitor only needs to deal with expressions, so this patch has
it implementExpressionVisitor. This patch also adds some missing test
cases to its unit test for handling unsupported type constructors.
Finally, we simplify the tests a bit by having them just parse expressions
(instead of whole statements).

This patch depends on #3126, so review that first.

@rodesai rodesai requested a review from a team as a code owner July 25, 2019 19:32
@rodesai rodesai force-pushed the sql-to-java-visitor-only-expressions branch 2 times, most recently from 7f93e1c to cdaf956 Compare July 26, 2019 03:07
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

two typos, otherwise LGTM!

@agavra agavra requested a review from a team July 26, 2019 19:59
SqlToJavaVisitor only needs to deal with expressions, so this patch has
it implementExpressionVisitor. This patch also adds some missing test
cases to its unit test for handling unsupported type constructors.
Finally, we simplify the tests a bit by having them just parse expressions
(instead of whole statements).
@rodesai rodesai force-pushed the sql-to-java-visitor-only-expressions branch from e671b4a to 8f29b9b Compare July 30, 2019 02:41
@rodesai rodesai merged commit 1efc467 into confluentinc:master Jul 30, 2019
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.

2 participants