Skip to content

Conversation

tswast
Copy link
Contributor

@tswast tswast commented Sep 7, 2017

@tswast tswast requested a review from andrewsg September 7, 2017 20:29
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 7, 2017
@tswast tswast requested a review from davidcavazos September 8, 2017 19:24

args = parser.parse_args()

if args.use_standard_sql:
query_standard_sql(args.query)
elif args.destination_table:
dataset, table = args.destination_table.split('.')
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use a simple regular expression to validate that the destination table name is in the correct format, i.e. dataset_name.table_name

Copy link
Contributor

Choose a reason for hiding this comment

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

However, this being a sample, it might be a better idea to keep unnecessary complexity down. So I'll leave that to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather keep the complexity low.

@davidcavazos
Copy link
Contributor

Please do whatever you think best for the validation comment, otherwise LGTM

# Allow the results table to be overwritten.
query_job.write_disposition = 'WRITE_TRUNCATE'

query_job.begin()
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't .result() call .begin()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't realize that. I can update the sample if so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants