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: unsetting clustering fields on Table is now possible #622

Merged
merged 5 commits into from Apr 26, 2021

Conversation

@plamut
Copy link
Contributor

@plamut plamut commented Apr 21, 2021

Closes #615.

Support for mutable clustering configuration already existed, but due to a bug it was not possible to unset the clustering fields altogether. This PR fixes that, and makes some refactoring changes to table.py.

PR checklist:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)
    • "Documentation reflects this capability, and possibly links to the cloud.google.com content above to describe the behavior when clustering is added/changed." - DONE in #330 (samples using clustering on a non-partitioned table).
    • "Stale comments indicating that clustering requires a partitioning config are removed (you can cluster a table without partitioning it)" - DONE in #286
@plamut plamut changed the title fix: unsetting clustering fileds on Table is now possile fix: unsetting clustering fileds on Table is now possible Apr 22, 2021
@plamut plamut marked this pull request as ready for review Apr 22, 2021
@plamut plamut requested a review from as a code owner Apr 22, 2021
@plamut plamut requested review from steffnay, tswast and shollyman and removed request for and steffnay Apr 22, 2021
Copy link
Contributor

@tswast tswast left a comment

Sweet! Just one suggestion, but otherwise looks good. Thanks for adding that system test.

Loading

google/cloud/bigquery/table.py Show resolved Hide resolved
Loading
@@ -316,17 +329,23 @@ def __init__(self, table_ref, schema=None):
@property
def project(self):
"""str: Project bound to the table."""
return self._properties["tableReference"]["projectId"]
return _helpers._get_sub_prop(
self._properties, self._PROPERTY_TO_API_FIELD["project"]
Copy link
Contributor

@tswast tswast Apr 23, 2021

Choose a reason for hiding this comment

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

Ah! That makes sense. I bet we could go one level further and do some metaclass magic to generate properties, but metaclasses are one level of magic too far in my opinion. This looks good to me.

Loading

Copy link
Contributor Author

@plamut plamut Apr 23, 2021

Choose a reason for hiding this comment

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

I always thought a custom dict subtype could be useful here, but never convinced myself that we really need it - even though it would be less arcane than using metaclasses.

Proof of concept (untested):

class ApiProperties(dict):
    def __getitem__(self, key):
        if not isinstance(key, tuple):
            return super().__getitem__(key)
        result = super()
        for item in key:
            result = result.__getitem__(item)
        return result

>>> p = ApiProperties({"foo": {"subfoo": "bar"}})
>>> p
{'foo': {'subfoo': 'bar'}}
>>> p["foo"]["subfoo"]
'bar'
>>> p["foo", "subfoo"]  # also works, without _helpers
'bar'

The customized dict would assume that the keys are never tuples (which is true, we just use strings), and it could override __setitem__() in a similar fashion. An it could also automatically create subdictionaries if a (sub) key does not exist yet, similarly to defaultdict. And more...

We could consider this a a possible P3 improvement at some point in the future...

Edit: Ah, you mean generating the properties on the basis of _PROPERTY_TO_API_FIELD declarations? That would be fun, but probably indeed too magical.

Loading

Copy link
Contributor

@tswast tswast Apr 26, 2021

Choose a reason for hiding this comment

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

I hadn't thought about subclassing what we use for dictionaries. That would indeed help clean thing up a bit, assuming it's still JSON-serializable.

Loading

google/cloud/bigquery/table.py Show resolved Hide resolved
Loading
@plamut plamut requested a review from tswast Apr 26, 2021
tswast
tswast approved these changes Apr 26, 2021
@tswast tswast merged commit 33a871f into googleapis:master Apr 26, 2021
10 checks passed
Loading
@tswast tswast changed the title fix: unsetting clustering fileds on Table is now possible fix: unsetting clustering fields on Table is now possible Apr 26, 2021
@plamut plamut deleted the iss-615 branch Apr 26, 2021
gcf-merge-on-green bot pushed a commit that referenced this issue Apr 26, 2021
🤖 I have created a release \*beep\* \*boop\*
---
## [2.14.0](https://www.github.com/googleapis/python-bigquery/compare/v2.13.1...v2.14.0) (2021-04-26)


### Features

* accept DatasetListItem where DatasetReference is accepted ([#597](https://www.github.com/googleapis/python-bigquery/issues/597)) ([c8b5581](https://www.github.com/googleapis/python-bigquery/commit/c8b5581ea3c94005d69755c4a3b5a0d8900f3fe2))
* accept job object as argument to `get_job` and `cancel_job` ([#617](https://www.github.com/googleapis/python-bigquery/issues/617)) ([f75dcdf](https://www.github.com/googleapis/python-bigquery/commit/f75dcdf3943b87daba60011c9a3b42e34ff81910))
* add `Client.delete_job_metadata` method to remove job metadata ([#610](https://www.github.com/googleapis/python-bigquery/issues/610)) ([0abb566](https://www.github.com/googleapis/python-bigquery/commit/0abb56669c097c59fbffce007c702e7a55f2d9c1))
* add `max_queue_size` argument to `RowIterator.to_dataframe_iterable` ([#575](https://www.github.com/googleapis/python-bigquery/issues/575)) ([f95f415](https://www.github.com/googleapis/python-bigquery/commit/f95f415d3441b3928f6cc705cb8a75603d790fd6))
* add type hints for public methods ([#613](https://www.github.com/googleapis/python-bigquery/issues/613)) ([f8d4aaa](https://www.github.com/googleapis/python-bigquery/commit/f8d4aaa335a0eef915e73596fc9b43b11d11be9f))
* DB API cursors are now iterable ([#618](https://www.github.com/googleapis/python-bigquery/issues/618)) ([e0b373d](https://www.github.com/googleapis/python-bigquery/commit/e0b373d0e721a70656ed8faceb7f5c70f642d144))
* retry google.auth TransportError by default ([#624](https://www.github.com/googleapis/python-bigquery/issues/624)) ([34ecc3f](https://www.github.com/googleapis/python-bigquery/commit/34ecc3f1ca0ff073330c0c605673d89b43af7ed9))
* use pyarrow stream compression, if available ([#593](https://www.github.com/googleapis/python-bigquery/issues/593)) ([dde9dc5](https://www.github.com/googleapis/python-bigquery/commit/dde9dc5114c2311fb76fafc5b222fff561e8abf1))


### Bug Fixes

* consistent percents handling in DB API query ([#619](https://www.github.com/googleapis/python-bigquery/issues/619)) ([6502a60](https://www.github.com/googleapis/python-bigquery/commit/6502a602337ae562652a20b20270949f2c9d5073))
* missing license headers in new test files ([#604](https://www.github.com/googleapis/python-bigquery/issues/604)) ([df48cc5](https://www.github.com/googleapis/python-bigquery/commit/df48cc5a0be99ad39d5835652d1b7422209afc5d))
* unsetting clustering fileds on Table is now possible ([#622](https://www.github.com/googleapis/python-bigquery/issues/622)) ([33a871f](https://www.github.com/googleapis/python-bigquery/commit/33a871f06329f9bf5a6a92fab9ead65bf2bee75d))


### Documentation

* add sample to run DML query ([#591](https://www.github.com/googleapis/python-bigquery/issues/591)) ([ff2ec3a](https://www.github.com/googleapis/python-bigquery/commit/ff2ec3abe418a443cd07751c08e654f94e8b3155))
* update the description of the return value of `_QueryResults.rows()` ([#594](https://www.github.com/googleapis/python-bigquery/issues/594)) ([8f4c0b8](https://www.github.com/googleapis/python-bigquery/commit/8f4c0b84dac3840532d7865247b8ad94b625b897))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants