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: DB API depends on pyarrow when decimal query parameters are used #551

Merged
merged 3 commits into from Mar 16, 2021

Conversation

plamut
Copy link
Contributor

@plamut plamut commented Mar 15, 2021

Fixes #549.

This PR removes the hard dependency of DB API on the optional pyarrow dependency.

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)

DB API should gracefully handle the case when the optional pyarrow
dependency is not installed.
@plamut plamut requested review from tswast and a team March 15, 2021 11:44
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 15, 2021
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Mar 15, 2021
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.

Based on my latest comment, I think we can remove pyarrow entirely from this logic.

Comment on lines +192 to 201
# NUMERIC values have precision of 38 (number of digits) and scale of 9 (number
# of fractional digits), and their max absolute value must be strictly smaller
# than 1.0E+29.
# https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#decimal_types
if (
len(vtuple.digits) <= 38 # max precision: 38
and vtuple.exponent >= -9 # max scale: 9
and _NUMERIC_SERVER_MIN <= value <= _NUMERIC_SERVER_MAX
):
return "NUMERIC"
Copy link
Contributor Author

@plamut plamut Mar 16, 2021

Choose a reason for hiding this comment

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

The logic differs from the docs, because testing the actual backend response showed that 9.9999999999999999999999999999999999999E+29 is not accepted as a valid NUMERIC value:

SELECT CAST((SELECT "9.9999999999999999999999999999999999999E+29" AS literal) AS NUMERIC);  # error

Even 1.0E+29 is too large, but 9.999...9E+28 works, meaning that there's probably an off-by-one error in the docs for the exponent.

Update: Confirmed, the docs are indeed not correct, an internal issue 182900100 has been created to track this.

@plamut plamut requested review from shollyman and tswast March 16, 2021 10:49
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.

Gorgeous!

@tswast tswast merged commit 1b946ba into googleapis:master Mar 16, 2021
@plamut plamut deleted the iss-549 branch March 16, 2021 15:56
@@ -71,6 +72,33 @@ def test_scalar_to_query_parameter(self):
self.assertEqual(named_parameter.type_, expected_type, msg=msg)
self.assertEqual(named_parameter.value, value, msg=msg)

def test_decimal_to_query_parameter(self): # TODO: merge with previous test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should have removed that, it's just a subset of the test_scalar_to_query_parameter test above it.

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 googleapis/python-bigquery API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DB-API depends on pyarrow when decimal query parameters are used
2 participants