feat: add roundingmode enum, wiring, and tests #2121
Conversation
product-auto-label
bot
added
size: m
google/cloud/bigquery/enums.py
Outdated
| * 2.5 => 2 | ||
| """ | ||
|
|
||
| ROUNDING_MODE_UNSPECIFIED = 0 |
There was a problem hiding this comment.
I'm curious, is this what the REST API uses? In past, the BQ enums mapped to strings, even if they start life in proto as integers.
Per https://protobuf.dev/programming-guides/json/, supposedly both should be accepted, but I think we should use whatever the JSON API returns so customers' equality checks function.
There was a problem hiding this comment.
The REST API is as follows:
"roundingMode": {
"description": "Optional. Specifies the rounding mode to be used when storing values of NUMERIC and BIGNUMERIC type.",
"enum": [
"ROUNDING_MODE_UNSPECIFIED",
"ROUND_HALF_AWAY_FROM_ZERO",
"ROUND_HALF_EVEN"
],
"enumDescriptions": [
"Unspecified will default to using ROUND_HALF_AWAY_FROM_ZERO.",
"ROUND_HALF_AWAY_FROM_ZERO rounds half values away from zero when applying precision and scale upon writing of NUMERIC and BIGNUMERIC values. For Scale: 0 1.1, 1.2, 1.3, 1.4 =\u003e 1 1.5, 1.6, 1.7, 1.8, 1.9 =\u003e 2",
"ROUND_HALF_EVEN rounds half values to the nearest even value when applying precision and scale upon writing of NUMERIC and BIGNUMERIC values. For Scale: 0 1.1, 1.2, 1.3, 1.4 =\u003e 1 1.5 =\u003e 2 1.6, 1.7, 1.8, 1.9 =\u003e 2 2.5 =\u003e 2"
],
"type": "string"
google/cloud/bigquery/enums.py
Outdated
| @@ -344,3 +358,29 @@ class DeterminismLevel: | |||
|
|
|||
| NOT_DETERMINISTIC = "NOT_DETERMINISTIC" | |||
| """The UDF is not deterministic.""" | |||
|
|
|||
|
|
|||
| class RoundingMode(enum.Enum): | |||
There was a problem hiding this comment.
I believe if we do a class RoundingMode(str, enum.Enum): with enum.auto() values as above in StandardSqlTypeNames, this will give strings compatible with the BQ REST API (assuming these work like other enums in that regard).
google/cloud/bigquery/schema.py
Outdated
| # The order of operations is important: | ||
| # If field_type is FOREIGN, then foreign_type_definition must be set. | ||
| if field_type != "FOREIGN": | ||
| self._properties["type"] = field_type |
There was a problem hiding this comment.
This looks redundant with line 187/220 above ("type": field_type,) I don't think this is necessary.
google/cloud/bigquery/schema.py
Outdated
| if isinstance(rounding_mode, enums.RoundingMode): | ||
| rounding_mode = rounding_mode.name |
There was a problem hiding this comment.
I think if we do the RoundingMode(str, enum.Enum) trick, this logic should be unnecessary.
Would be good to double check in some unit tests that json.dumps(field.to_api_repr()) works when passing in one of these enums if we do remove this if statement.
There was a problem hiding this comment.
With the changes to the Rounding_Mode enum and our existing test suite, we do have a test that ensures given an enum representation, we get the correct JSON output when calling to_api_repr(). Specifically, see test_to_api_repr().
google/cloud/bigquery/schema.py
Outdated
| raise ValueError( | ||
| "If the 'field_type' is 'FOREIGN', then 'foreign_type_definition' is required." | ||
| ) |
There was a problem hiding this comment.
I'm wary of putting client-side validation like this, since theoretically the server could choose to lift this restriction in future (imagine some foreign schema autodetect support or something).
tests/unit/test_schema.py
Outdated
| @@ -80,6 +86,8 @@ def test_constructor_explicit(self): | |||
| ) | |||
| ), | |||
| ) | |||
| self.assertEqual(field.rounding_mode, ROUNDINGMODE.name) | |||
There was a problem hiding this comment.
Nit: The string "ROUNDING_MODE_UNSPECIFIED" would be easier to convince myself that the code is correct. With .name, this is a bit of a "change detector test", since we've duplicated that logic from the implementation.
google/cloud/bigquery/schema.py
Outdated
| @@ -206,6 +240,11 @@ def __init__( | |||
| self._properties["rangeElementType"] = {"type": range_element_type} | |||
| if isinstance(range_element_type, FieldElementType): | |||
| self._properties["rangeElementType"] = range_element_type.to_api_repr() | |||
| if rounding_mode is not None: | |||
| self._properties["roundingMode"] = rounding_mode | |||
| if isinstance(foreign_type_definition, str): | |||
There was a problem hiding this comment.
I'm curious, why not is not None here?
chalmerlowe
deleted the
feat-b358215039-add-roundingmode-enum-and-wiring
branch
This PR adds the the rounding_mode enum, support for the foreign_type_definition, some wiring between those items and various classes and functions and the associated tests.