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

Fix new partitioned table #3381

Merged
merged 3 commits into from
May 15, 2017

Conversation

richkadel
Copy link
Contributor

@richkadel richkadel commented May 6, 2017

[This is a re-do of the pull request #3366 to fix the author email address to use my google.com email]

I moved the partitioning_type check into the if / elif block to avoid a ValueError that should not have been thrown.

See:
https://cloud.google.com/bigquery/docs/creating-partitioned-tables#creating_a_partitioned_table and the "API" example.

Before my change, the following code failed with the ValueError thrown by this function, but with my change, I was able to successfully create the partitioned table:

destination_table = dataset.table(table_shortname)

if not destination_table.exists(): 
    destination_table.partitioning_type = 'DAY'
    destination_table.create()

# Use a table partition
destination_partition = dataset.table(table_shortname + '$' + partition_date)

richkadel added 2 commits May 6, 2017 10:34
[This is a re-do of the pull request googleapis#3366 to fix the author email
address to use my google.com email]

I moved the partitioning_type check into the if/elif block to avoid a
ValueError that should not have been thrown.

See:
https://cloud.google.com/bigquery/docs/creating-partitioned-tables#creat
ing_a_partitioned_table and the "API" example.

Before my change, the following code failed with the ValueError thrown
by this function, but with my change, I was able to successfully create
the partitioned table:

destination_table = dataset.table(table_shortname)
destination_partition =
dataset.table(table_shortname+'$'+partition_date)

if not destination_table.exists():
  destination_table.partitioning_type = 'DAY'
  destination_table.create()
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 6, 2017
@richkadel
Copy link
Contributor Author

@dhermes @jonparrott
I resubmitted my changes in a new pull request to make sure it has my @google.com email, and withdrew pull request #3366

@dhermes
Copy link
Contributor

dhermes commented May 8, 2017

Thanks @richkadel. This LGTM.

@tswast Any issues?

@dhermes
Copy link
Contributor

dhermes commented May 8, 2017

@richkadel Your code snippet above never uses destination_partition. Is there a typo somewhere?

raise ValueError("Set either 'view_query' or 'schema'.")
elif self.partitioning_type is None:
raise ValueError(
"Set either 'view_query' or 'schema' or 'partitioning_type'.")

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@richkadel
Copy link
Contributor Author

@dhermes re: @richkadel Your code snippet above never uses destination_partition. Is there a typo somewhere?

No typo. The code that uses the partition itself is not shown. I left the line in to show that the table name used in the "create" call is the table name without the partition suffix. Once the table is created then you can use destination_partition value as the "job_destination" setting to materialize data into just that partition.

@dhermes
Copy link
Contributor

dhermes commented May 8, 2017

@richkadel Gotcha, I just made an edit to make that clearer.

As discussed with team, I removed the  ValueError and any assumptions
about the legitimate combinations of values in the request.

ValueError unit test was removed, and the unused schema in
`test_create_w_alternate_client` that was suppressed due to the `elif`
in `_build_resource`, was removed so the expected value still matches
the remaining inputs.
@richkadel
Copy link
Contributor Author

Updates have been pushed and all tests pass.

Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

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

Thank you!

@dhermes
Copy link
Contributor

dhermes commented May 9, 2017

OK @jonparrott also thinks this looks just fine.

@tseaver tseaver added api: bigquery Issues related to the BigQuery API. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels May 15, 2017
@tseaver tseaver merged commit 5bc1e38 into googleapis:master May 15, 2017
@richkadel richkadel deleted the fix-new-partitioned-table branch May 18, 2017 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API. cla: yes This human has signed the Contributor License Agreement. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants