feat: Extended DB API parameter syntax to optionally provide parameter types #626

jimfulton · 2021-04-26T15:23:12Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

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)

Fixes #609 🦕

plamut

Not a complete review by any means, just glanced over the PR and the regex pattern caught my eye, gave have two improvement suggestions.

google/cloud/bigquery/dbapi/cursor.py

…Thanks Peter

…expressions.

tswast · 2021-04-27T21:05:07Z

google/cloud/bigquery/dbapi/_helpers.py

@@ -26,9 +26,42 @@

 _NUMERIC_SERVER_MIN = decimal.Decimal("-9.9999999999999999999999999999999999999E+28")
 _NUMERIC_SERVER_MAX = decimal.Decimal("9.9999999999999999999999999999999999999E+28")
+_BIGQUERY_SCALAR_TYPES = {


Any chance we can use

python-bigquery/google/cloud/bigquery/enums.py

Line 219 in 4396e70

class SqlParameterScalarTypes:

or

python-bigquery/google/cloud/bigquery/enums.py

Line 145 in 4396e70

_SQL_SCALAR_TYPES = frozenset(

instead? I worry this will get out-of-sync (already has, missing GEOGRAPHY, which just encodes as a string)

Yes! I didn't know those existed.

So some questions:

SqlParameterScalarTypes includes some aliases.
Is that something we want in this context? (If so, I'll lobby for INT. ;))

SqlParameterScalarTypes and _SQL_SCALAR_TYPES lack DECIMAL and BIGDECIMAL.
https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#decimal_types

Should I add DECIMAL and BIGDECIMAL?

SqlParameterScalarTypes and _SQL_SCALAR_TYPES lack DECIMAL and BIGDECIMAL.
https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#decimal_types
Should I add DECIMAL and BIGDECIMAL?

Good catch! Filed #633 so that we can follow-up and add those.

SqlParameterScalarTypes includes some aliases.
Is that something we want in this context? (If so, I'll lobby for INT. ;))

Those aliases were intended to help folks who first learned BigQuery with "legacy" syntax. https://cloud.google.com/bigquery/data-types I think it's worth allowing the same aliases here in the DB-API.

On second-thought, I don't know if we want to be adding DECIMAL and BIGDECIMAL as aliases. Let's leave them out of this implementation for now, and we can decide later if we want more aliases beyond those we have for Legacy SQL support.

But they're documented in the standard sql docs.

google/cloud/bigquery/dbapi/_helpers.py

tswast · 2021-04-27T21:16:31Z

google/cloud/bigquery/dbapi/cursor.py

-            self.execute(operation, parameters)
+        if seq_of_parameters:
+            formatted_operation, parameter_types = _format_operation(
+                operation, seq_of_parameters[0]


Could you describe in a comment why we grab just the first one? I guess either we get the types correct on the first go, or we have a problem anyway?

Also it's a niche case, but it's possible that someone could pass in a generator or something for the sequence of parameters. In which case [0] won't work. I think we need to either always loop like we had (and add some logic to only extract the types once) or do some funny business with the low-level iteration function.

~~I probably should have had a test case with this (generator as parameters) when I documented the allowed type as a Sequence.~~

Edit: Nevermind, Sequence supports integer-based item access https://docs.python.org/3/glossary.html#term-sequence It's "Iterable" that only supports iteration. We might actually want to support this use case, but probably best to wait until we actually get a customer request for that feature.

Comment added.

Co-authored-by: Tim Swast <swast@google.com>

google/cloud/bigquery/dbapi/cursor.py

tswast

Looks good!

One optional suggestion, but I'm okay with solving the type aliases feature at a future date (if ever)

tswast · 2021-04-28T15:09:26Z

tests/unit/test_dbapi_cursor.py

+            ("values('%%(oof:INT64)s, %(foo)s, %(foo)s)", dict(foo="INT64")),
+        ),
+        (
+            "values(%(foo:INT64)s, %(foo:INT64)s)",


Can we add some tests that check our aliases, too? (INTEGER -> INT64, for example)

I think there is a backend alias for INTEGER, so it's not strictly necessary, but will be if we want to support aliases like DECIMAL in the future.

That's included in my latest push.

We do need it if we want to do client-side checks.

google/cloud/bigquery/dbapi/_helpers.py

plamut

Looks good, although I mostly focused on the params replacement logic and only glanced over the rest (but didn't see anything obvious to fix).

🤖 I have created a release \*beep\* \*boop\* --- ## [2.15.0](https://www.github.com/googleapis/python-bigquery/compare/v2.14.0...v2.15.0) (2021-04-29) ### Features * Extended DB API parameter syntax to optionally provide parameter types ([#626](https://www.github.com/googleapis/python-bigquery/issues/626)) ([8bcf397](https://www.github.com/googleapis/python-bigquery/commit/8bcf397fbe2527e06317741875a059b109cfcd9c)) ### Bug Fixes * add DECIMAL and BIGDECIMAL as aliases for NUMERIC and BIGNUMERIC ([#638](https://www.github.com/googleapis/python-bigquery/issues/638)) ([aa59023](https://www.github.com/googleapis/python-bigquery/commit/aa59023317b1c63720fb717b3544f755652da58d)) * The DB API Binary function accepts bytes data ([#630](https://www.github.com/googleapis/python-bigquery/issues/630)) ([4396e70](https://www.github.com/googleapis/python-bigquery/commit/4396e70771af6889d3242c37c5ff2e80241023a2)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

jimfulton added 5 commits April 22, 2021 10:22

Added daft explicit type documentation.

f420e71

Extended query-parameter system for specifying parameter types.

c33cc7f

simplified expression because query parameter types are always passed.

7a60a4e

Serialize non-floats (e.g. Decimals) using in FLOAT64 parameters.

89ed86e

remove excess blank lines.

d91105d

jimfulton requested review from a team and tswast and removed request for a team April 26, 2021 15:23

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 26, 2021

product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Apr 26, 2021

tswast added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 26, 2021

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 26, 2021

jimfulton changed the title ~~feat: Extended DB API parameter systax to optionally provide parameter types~~ feat: Extended DB API parameter syntax to optionally provide parameter types Apr 26, 2021

plamut reviewed Apr 27, 2021

View reviewed changes

google/cloud/bigquery/dbapi/cursor.py Outdated Show resolved Hide resolved

google/cloud/bigquery/dbapi/cursor.py Outdated Show resolved Hide resolved

jimfulton added 4 commits April 27, 2021 14:21

parameterized the _extract_types tests.

9f2f189

Fixed regex used by _extract_types to not get fooled by double %s -- …

a8329f3

…Thanks Peter

Merge master

7a35ed5

Changed regex used in _extract_types to not capture uninteresting sub…

337dcb2

…expressions.

tswast reviewed Apr 27, 2021

View reviewed changes

jimfulton and others added 4 commits April 27, 2021 15:37

blacken

43c19c1

Missing space

1dce435

Co-authored-by: Tim Swast <swast@google.com>

added a comment explaining why we only use the first set of parameters.

97e020f

merged origin

5110653

plamut reviewed Apr 28, 2021

View reviewed changes

google/cloud/bigquery/dbapi/cursor.py Outdated Show resolved Hide resolved

Handle multiple %s before parameter placeholders in _extract_types

13c4a3a

tswast mentioned this pull request Apr 28, 2021

feat: add DECIMAL and BIGDECIMAL as aliases for NUMERIC and BIGNUMERIC to scalar types enums #633

Closed

jimfulton added 2 commits April 28, 2021 09:08

Use enums.SqlParameterScalarTypes for checking type validity

6e496fb

blacken

2173226

tswast approved these changes Apr 28, 2021

View reviewed changes

Dereference aliases in SqlParameterScalarTypes when checking types

86b243c

jimfulton added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 28, 2021

tswast approved these changes Apr 28, 2021

View reviewed changes

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 28, 2021

plamut approved these changes Apr 28, 2021

View reviewed changes

jimfulton merged commit 8bcf397 into master Apr 29, 2021

jimfulton deleted the riversnake-cursor-types-from-query branch April 29, 2021 13:19

jimfulton mentioned this pull request Apr 29, 2021

Fix: Add DECIMAL and BIGDECIMAL as aliases for NUMERIC and BIGNUMERIC #638

Merged

4 tasks

release-please bot mentioned this pull request Jan 4, 2022

chore(main): release python-bigquery 1.27.1 #1097

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Extended DB API parameter syntax to optionally provide parameter types #626

feat: Extended DB API parameter syntax to optionally provide parameter types #626

jimfulton commented Apr 26, 2021

plamut left a comment

tswast Apr 27, 2021

jimfulton Apr 27, 2021

tswast Apr 28, 2021

tswast Apr 28, 2021

jimfulton Apr 28, 2021

tswast Apr 27, 2021

jimfulton Apr 27, 2021

tswast left a comment

tswast Apr 28, 2021

tswast Apr 28, 2021

jimfulton Apr 28, 2021

plamut left a comment

feat: Extended DB API parameter syntax to optionally provide parameter types #626

feat: Extended DB API parameter syntax to optionally provide parameter types #626

Conversation

jimfulton commented Apr 26, 2021

plamut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tswast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plamut left a comment

Choose a reason for hiding this comment