-
Notifications
You must be signed in to change notification settings - Fork 301
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
feat: add support for decimal target types #735
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
|
||
import base64 | ||
import copy | ||
from typing import FrozenSet, Iterable, Optional | ||
|
||
from google.cloud.bigquery._helpers import _to_bytes | ||
from google.cloud.bigquery._helpers import _bytes_to_json | ||
|
@@ -693,6 +694,28 @@ def compression(self): | |
def compression(self, value): | ||
self._properties["compression"] = value | ||
|
||
@property | ||
def decimal_target_types(self) -> Optional[FrozenSet[str]]: | ||
"""Possible SQL data types to which the source decimal values are converted. | ||
|
||
See: | ||
https://cloud.google.com/bigquery/docs/reference/rest/v2/tables#ExternalDataConfiguration.FIELDS.decimal_target_types | ||
|
||
.. versionadded:: 2.21.0 | ||
""" | ||
prop = self._properties.get("decimalTargetTypes") | ||
if prop is not None: | ||
prop = frozenset(prop) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The enum docstrings added above suggest that order is significant. However, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per docs, the order is not significant, which why a (frozen) set is returned.
The backend always uses the following order: NUMERIC, BIGNUMERIC, and STRING (among the target types provided). I'll re-read the enum docstrings and make them more clear, if necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we are storing it as a list in the setter for compatibility with how the back-end sends it to us? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's primarily because sets are not JSON serializable out of the box - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shollyman The Should the generated docs follow suit? Or perhaps modify the wording? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi Peter, yes I can see how the doc contradicts the disco doc description. Checking w backend guy right now. Will update shortly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "disco doc" - gotta love that lingo. ❤️ 🎊 🤣 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Upon checking w myzhong@, the order actually does not matter here. It would always go for NUMERIC if it's possible even if user specified ["BIGNUMERIC", "NUMERIC"]. https://cloud.google.com/bigquery/docs/reference/rest/v2/Job says "The order of the types in this field is ignored" and is accurate. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK thanks, we'll keep the target types in a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. b/193235792 created to track the doc update. cl/383854927 created to update the doc. Pending review from BQ techwriters. |
||
return prop | ||
|
||
@decimal_target_types.setter | ||
def decimal_target_types(self, value: Optional[Iterable[str]]): | ||
if value is not None: | ||
self._properties["decimalTargetTypes"] = list(value) | ||
else: | ||
if "decimalTargetTypes" in self._properties: | ||
del self._properties["decimalTargetTypes"] | ||
|
||
@property | ||
def hive_partitioning(self): | ||
"""Optional[:class:`~.external_config.HivePartitioningOptions`]: [Beta] When set, \ | ||
|
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.
I notice we are missing docs for a bunch of enums. I filed #744 to switch the docs over to module-based docs for this module.