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)
plamut added 2 commits Mar 15, 2021
DB API should gracefully handle the case when the optional pyarrow
dependency is not installed.
@plamut plamut requested a review from tswast Mar 15, 2021
@plamut plamut requested a review from as a code owner Mar 15, 2021
@google-cla google-cla bot added the cla: yes label Mar 15, 2021
google/cloud/bigquery/dbapi/_helpers.py Outdated Show resolved Hide resolved
Loading
Copy link
Contributor

@tswast tswast left a comment

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

Loading

# 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.

Loading

@plamut plamut requested review from shollyman and tswast Mar 16, 2021
tswast
tswast approved these changes Mar 16, 2021
Copy link
Contributor

@tswast tswast left a comment

Gorgeous!

Loading

@tswast tswast merged commit 1b946ba into googleapis:master Mar 16, 2021
10 checks passed
Loading
@plamut plamut deleted the iss-549 branch Mar 16, 2021
@@ -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

@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.

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

Loading

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