- Notifications
You must be signed in to change notification settings - Fork 321
feat: support timestamp_precision in table schema #2333
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
base: main
Are you sure you want to change the base?
Changes from all commits
2e1daa8 253ac1f dc3c498 97f0251 234a3fd b20159b a1bc2cb bc6dcda 518a12c a8d5f5c 0567adf 1268c45 8603973 cb9f818 696dfff d24df7d 873bff6 6a93c26 9a4f72f c146e39 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 |
|---|---|---|
| | @@ -196,6 +196,16 @@ class SchemaField(object): | |
| | ||
| Only valid for top-level schema fields (not nested fields). | ||
| If the type is FOREIGN, this field is required. | ||
| | ||
| timestamp_precision: Optional[int] | ||
| Precision (maximum number of total digits in base 10) for seconds | ||
| of TIMESTAMP type. | ||
| | ||
| Possible values include: | ||
| | ||
| - None (Default, for TIMESTAMP type with microsecond precision) | ||
| | ||
| - 12 (For TIMESTAMP type with picosecond precision) | ||
| """ | ||
| | ||
| def __init__( | ||
| | @@ -213,6 +223,7 @@ def __init__( | |
| range_element_type: Union[FieldElementType, str, None] = None, | ||
| rounding_mode: Union[enums.RoundingMode, str, None] = None, | ||
| foreign_type_definition: Optional[str] = None, | ||
| timestamp_precision: Optional[int] = None, | ||
| ): | ||
| self._properties: Dict[str, Any] = { | ||
| "name": name, | ||
| | @@ -237,6 +248,8 @@ def __init__( | |
| if isinstance(policy_tags, PolicyTagList) | ||
| else None | ||
| ) | ||
| if timestamp_precision is not None: | ||
| self._properties["timestampPrecision"] = timestamp_precision | ||
| if isinstance(range_element_type, str): | ||
| self._properties["rangeElementType"] = {"type": range_element_type} | ||
| if isinstance(range_element_type, FieldElementType): | ||
| | @@ -374,6 +387,19 @@ def policy_tags(self): | |
| resource = self._properties.get("policyTags") | ||
| return PolicyTagList.from_api_repr(resource) if resource is not None else None | ||
| | ||
| @property | ||
| def timestamp_precision(self): | ||
| """Precision (maximum number of total digits in base 10) for seconds of | ||
| TIMESTAMP type. | ||
| | ||
| Possible values include: | ||
| | ||
| - None (Default, for TIMESTAMP type with microsecond precision) | ||
| | ||
| - 12 (For TIMESTAMP type with picosecond precision) | ||
| Contributor 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. No mention of None? (It says 6 is the default. Is the server enforcing that, or the client? Can this even return None?) Contributor Author 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 server does return None, if we do not set the value. This docstring is copied from the proto files. I just tried to set value to 6, and received the following error: So here we might need to give up consistency with the proto and make sure the doc is user friendly. WDYT? I will also open a bug with the API team. Contributor Author 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. opened internal bug 463739109 and updated docstring. Contributor 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. Usually in protos, None is equivalent to 0. So I'd recommend removing the Optional here, and have it return 0 if unset. But yeah, we should make sure we understand the expected values here first | ||
| """ | ||
| return _helpers._int_or_none(self._properties.get("timestampPrecision")) | ||
| | ||
| def to_api_repr(self) -> dict: | ||
| """Return a dictionary representing this schema field. | ||
| | ||
| | @@ -417,6 +443,7 @@ def _key(self): | |
| self.description, | ||
| self.fields, | ||
| policy_tags, | ||
| self.timestamp_precision, | ||
| ) | ||
| | ||
| def to_standard_sql(self) -> standard_sql.StandardSqlField: | ||
| | @@ -467,10 +494,9 @@ def __hash__(self): | |
| return hash(self._key()) | ||
| | ||
| def __repr__(self): | ||
| key = self._key() | ||
| policy_tags = key[-1] | ||
| *initial_tags, policy_tags, timestamp_precision_tag = self._key() | ||
| policy_tags_inst = None if policy_tags is None else PolicyTagList(policy_tags) | ||
| adjusted_key = key[:-1] + (policy_tags_inst,) | ||
| adjusted_key = (*initial_tags, policy_tags_inst, timestamp_precision_tag) | ||
| return f"{self.__class__.__name__}{adjusted_key}" | ||
| | ||
| | ||
| | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| | @@ -74,6 +74,13 @@ | |
| bigquery.SchemaField("full_name", "STRING", mode="REQUIRED"), | ||
| bigquery.SchemaField("age", "INTEGER", mode="REQUIRED"), | ||
| ] | ||
| SCHEMA_PICOSECOND = [ | ||
| bigquery.SchemaField("full_name", "STRING", mode="REQUIRED"), | ||
| bigquery.SchemaField("age", "INTEGER", mode="REQUIRED"), | ||
| bigquery.SchemaField( | ||
| "time_pico", "TIMESTAMP", mode="REQUIRED", timestamp_precision=12 | ||
| ), | ||
| ] | ||
| CLUSTERING_SCHEMA = [ | ||
| bigquery.SchemaField("full_name", "STRING", mode="REQUIRED"), | ||
| bigquery.SchemaField("age", "INTEGER", mode="REQUIRED"), | ||
| | @@ -631,6 +638,19 @@ def test_create_table_w_time_partitioning_w_clustering_fields(self): | |
| self.assertEqual(time_partitioning.field, "transaction_time") | ||
| self.assertEqual(table.clustering_fields, ["user_email", "store_code"]) | ||
| | ||
| def test_create_tabl_w_picosecond_timestamp(self): | ||
| dataset = self.temp_dataset(_make_dataset_id("create_table")) | ||
| table_id = "test_table" | ||
| table_arg = Table(dataset.table(table_id), schema=SCHEMA_PICOSECOND) | ||
| self.assertFalse(_table_exists(table_arg)) | ||
| | ||
| table = helpers.retry_403(Config.CLIENT.create_table)(table_arg) | ||
| self.to_delete.insert(0, table) | ||
| | ||
| self.assertTrue(_table_exists(table)) | ||
| self.assertEqual(table.table_id, table_id) | ||
| self.assertEqual(table.schema, SCHEMA_PICOSECOND) | ||
| Contributor 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. can we have a test that reads back a timestamp, and makes sure its in the expected range? Or am I misunderstanding? | ||
| | ||
| def test_delete_dataset_with_string(self): | ||
| dataset_id = _make_dataset_id("delete_table_true_with_string") | ||
| project = Config.CLIENT.project | ||
| | ||
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.
should there be an enum or something for this?
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.
This is an option, I did not choose it because the backend defined it to be an integer, and I think we can let the backend handle value validation. What do you think?
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.
In other repos, this is something I'd define an enum for, and then accept either the enum or a raw int value
But I don't have too much context on this repo, so up to you if that makes sense here. Consistency with the rest of the code is probably more relevant