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

[BEAM-1082] Make the legacy SQL flag consistent between Java and Python #1497

Closed

Conversation

sb2nov
Copy link
Contributor

@sb2nov sb2nov commented Dec 3, 2016

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify. (Even better, enable
    Travis-CI on your fork and ensure the whole test matrix passes).
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

R: @chamikaramj @aaltay PTAL

@@ -374,7 +374,7 @@ def __init__(self, table=None, dataset=None, project=None, query=None,
self.use_legacy_sql = True
else:
self.query = query
self.use_legacy_sql = use_legacy_sql
self.use_legacy_sql = not use_standard_sql
Copy link
Member

Choose a reason for hiding this comment

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

create a JIRA issue to fix the internals later. It would make sense to use use_standard_sql internally as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

useLegacySql: Specifies whether to use BigQuery's legacy
SQL dialect for this query. The default value is true. If set to false,
use_standard_sql: Specifies whether to use BigQuery's standard
SQL dialect for this query. The default value is False. If set to true,
Copy link
Member

Choose a reason for hiding this comment

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

capitalize True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@chamikaramj
Copy link
Contributor

Please add the JIRA to the PR title - [BEAM-873]

@sb2nov sb2nov changed the title Make the legacy SQL flag consistent between Java and Python [BEAM-1082] Make the legacy SQL flag consistent between Java and Python Dec 3, 2016
Sourabh Bajaj added 2 commits December 2, 2016 16:57
@sb2nov
Copy link
Contributor Author

sb2nov commented Dec 3, 2016

Thanks for the comments.

R: @chamikaramj @aaltay PTAL

@chamikaramj
Copy link
Contributor

LGTM

@sb2nov
Copy link
Contributor Author

sb2nov commented Dec 3, 2016

@robertwb please merge

@asfbot
Copy link

asfbot commented Dec 3, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5495/
--none--

@asfbot
Copy link

asfbot commented Dec 3, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5496/
--none--

@asfbot
Copy link

asfbot commented Dec 3, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/5497/
--none--

asfgit pushed a commit that referenced this pull request Dec 3, 2016
@sb2nov sb2nov closed this Dec 3, 2016
@sb2nov sb2nov deleted the BEAM-consistent-flag-bw-java-py branch December 3, 2016 07:00
@dhalperi
Copy link
Contributor

dhalperi commented Dec 3, 2016

The name of the BigQuery option is use_legacy_sql with default true. Why should Beam's interface differ?

Actually, this is probably reasonable from a user interface pov. Never mind ;)

As long as the flags inside of the connector are named use_legacy_sql so that the logic can be clearly understood in forming the JSON objects sent to the BQ api, I'm fine here.

@sb2nov
Copy link
Contributor Author

sb2nov commented Dec 3, 2016

@dhalperi Thanks for raising this. There is a follow up PR on minimizing the use_legacy_sql flag within the sdk. Do you have thoughts on it ?

#1499

@aaltay
Copy link
Member

aaltay commented Dec 3, 2016

@dhalperi Why is it reasonable from a user pov? Would not it make sense for external interfaces to match the underlying service (BQ)? (Either way the interface should be consistent across beam sdks)

@robertwb
Copy link
Contributor

robertwb commented Dec 6, 2016

I actually agree with @aaltay . I think both Python and Java should follow whatever BigQuery calls this feature in its publicly facing docs/APIs.

@sb2nov
Copy link
Contributor Author

sb2nov commented Dec 6, 2016

I think we can do two things here:

  1. Revert this and then change Java to use legacy_sql as a flag (https://cloud.google.com/bigquery/querying-data#bigquery-sync-query-java)
  2. Just let beam diverge from bigquery api docs.

I think 1 would have been better from the start but now it is a backward incompatible :/

@robertwb
Copy link
Contributor

robertwb commented Dec 6, 2016 via email

@sb2nov
Copy link
Contributor Author

sb2nov commented Dec 6, 2016

Would that still not be backward incompatible for java or is that okay ?

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.

6 participants