-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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-6080] Add support for bq single region dataset query #4815
[ZEPPELIN-6080] Add support for bq single region dataset query #4815
Conversation
@pan3793 can you please take a look. |
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.
Duplicate code should be avoided.
bigquery/src/main/java/org/apache/zeppelin/bigquery/BigQueryInterpreter.java
Show resolved
Hide resolved
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.
Much better, just small things
bigquery/src/main/java/org/apache/zeppelin/bigquery/BigQueryInterpreter.java
Show resolved
Hide resolved
bigquery/src/main/java/org/apache/zeppelin/bigquery/BigQueryInterpreter.java
Outdated
Show resolved
Hide resolved
bigquery/src/main/java/org/apache/zeppelin/bigquery/BigQueryInterpreter.java
Outdated
Show resolved
Hide resolved
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.
Code changes are looking good to me. Just the Pull request description should be updated.
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 except for the description @Reamer already mentioned.
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
@Reamer can you please merge the PR as well if it looks good. |
bigquery/src/main/java/org/apache/zeppelin/bigquery/BigQueryInterpreter.java
Outdated
Show resolved
Hide resolved
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.
Thanks, LGTM
What is this PR for?
A few sentences describing the overall goals of the pull request's commits.
First time? Check out the contributing guide - https://zeppelin.apache.org/contribution/contributions.html
What type of PR is it?
Improvement
Please leave your type of PR only
Todos
What is the Jira issue?
How should this be tested?
*CI
local tests
Screenshots (if appropriate)
Questions: