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

[ZEPPELIN-4512]. Move SqlSplitter from jdbc interpreter to zeppelin-interpreter #3569

Closed
wants to merge 1 commit into from

Conversation

zjffdu
Copy link
Contributor

@zjffdu zjffdu commented Dec 26, 2019

What is this PR for?

This PR move the sql split logic from jdbc interpreter to zeppelin-interpreter, so that other interpreter can reuse it. Besides that, it also fix some bugs of sql splitter, and add more unit test to verify it.

What type of PR is it?

[Bug Fix | Refactoring]

Todos

  • - Task

What is the Jira issue?

How should this be tested?

  • CI pass

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

The code looks good for me, but I have a general note. Some SQL dialects, also allows to use different comment characters. For example, MySQL allows to use # for single-line comments. Cassandra's SQL allows to use // for single line comments in addition to --. Maybe we can extend this code to handle an additional dialects as well? So it will be easier to reuse this split functionality in more interpreters?

@zjffdu
Copy link
Contributor Author

zjffdu commented Dec 27, 2019

Thanks for the comment @alexott
I have updated the PR to allow adding additional single line comment prefix. I am not sure whether's any other places needs to be customized for sql dialect.
SqlSplitterTest#testCustomSplitter_1 and SqlSplitterTest#testCustomSplitter_2 demonstrate that.

Copy link
Contributor

@alexott alexott 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 for addition of the custom comments @zjffdu !

@zjffdu
Copy link
Contributor Author

zjffdu commented Dec 28, 2019

Thanks @alexott Will merge if no more comment

@asfgit asfgit closed this in aea44a4 Dec 28, 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
2 participants