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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 17 commits into from Apr 29, 2021

Conversation

@jimfulton
Copy link
Contributor

@jimfulton jimfulton commented Apr 26, 2021

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 馃

@jimfulton jimfulton requested a review from as a code owner Apr 26, 2021
@jimfulton jimfulton requested review from tswast and removed request for Apr 26, 2021
@google-cla google-cla bot added the cla: yes label Apr 26, 2021
@jimfulton 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
Copy link
Contributor

@plamut plamut left a comment

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

Loading

google/cloud/bigquery/dbapi/cursor.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/dbapi/cursor.py Outdated Show resolved Hide resolved
Loading
@@ -26,9 +26,42 @@

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

@tswast tswast Apr 27, 2021

Choose a reason for hiding this comment

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

Any chance we can use

class SqlParameterScalarTypes:
or
_SQL_SCALAR_TYPES = frozenset(
instead? I worry this will get out-of-sync (already has, missing GEOGRAPHY, which just encodes as a string)

Loading

Copy link
Contributor Author

@jimfulton jimfulton Apr 27, 2021

Choose a reason for hiding this comment

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

Yes! I didn't know those existed.

So some questions:

Loading

Copy link
Contributor

@tswast tswast Apr 28, 2021

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor

@tswast tswast Apr 28, 2021

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@jimfulton jimfulton Apr 28, 2021

Choose a reason for hiding this comment

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

But they're documented in the standard sql docs.

Loading

google/cloud/bigquery/dbapi/_helpers.py Outdated Show resolved Hide resolved
Loading
self.execute(operation, parameters)
if seq_of_parameters:
formatted_operation, parameter_types = _format_operation(
operation, seq_of_parameters[0]
Copy link
Contributor

@tswast tswast Apr 27, 2021

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@jimfulton jimfulton Apr 27, 2021

Choose a reason for hiding this comment

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

Comment added.

Loading

google/cloud/bigquery/dbapi/cursor.py Outdated Show resolved Hide resolved
Loading
tswast
tswast approved these changes Apr 28, 2021
Copy link
Contributor

@tswast tswast left a comment

Looks good!

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

Loading

("values('%%(oof:INT64)s, %(foo)s, %(foo)s)", dict(foo="INT64")),
),
(
"values(%(foo:INT64)s, %(foo:INT64)s)",
Copy link
Contributor

@tswast tswast Apr 28, 2021

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor

@tswast tswast Apr 28, 2021

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@jimfulton jimfulton Apr 28, 2021

Choose a reason for hiding this comment

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

That's included in my latest push.

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

Loading

google/cloud/bigquery/dbapi/_helpers.py Show resolved Hide resolved
Loading
tswast
tswast approved these changes Apr 28, 2021
plamut
plamut approved these changes Apr 28, 2021
Copy link
Contributor

@plamut plamut left a comment

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

Loading

@jimfulton jimfulton merged commit 8bcf397 into master Apr 29, 2021
10 checks passed
Loading
@jimfulton jimfulton deleted the riversnake-cursor-types-from-query branch Apr 29, 2021
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.

4 participants