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: dry run queries with DB API cursor #128

Merged
merged 6 commits into from Jun 23, 2020
Merged

Conversation

@plamut
Copy link
Contributor

@plamut plamut commented Jun 10, 2020

Fixes #118.

This PR fixes the error in DB API cursor if a dry run query is run.

As a side effect, it also fixes handling the client's default query job config in DB API (the default config, if any, has been ignored to date).

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 plamut requested a review from shollyman Jun 10, 2020
google/cloud/bigquery/dbapi/cursor.py Outdated Show resolved Hide resolved
Loading
@shollyman
Copy link
Contributor

@shollyman shollyman commented Jun 10, 2020

I'm not as familiar with dbapi abstraction, but this seems like an odd pattern. I'd expect something like an empty row iterator and some form of side channel for getting metadata, but scanning https://www.python.org/dev/peps/pep-0249/ it doesn't seem like there's such an abstraction. Nor does it seem like there's an explicit prepare(), which is where you typically do things like validate queries.

My worry about doing things like building a custom schema and row is brittleness; if we add more metadata over time it seems like this change necessitates we keep widening the schema and keep the single-row semantics. Is this how other engines have hoisted prepare semantics into the dbapi abstraction?

Loading

@tswast
Copy link
Contributor

@tswast tswast commented Jun 10, 2020

This issue actually has bubbled it's way here from the SQLAlchemy connector. googleapis/python-bigquery-sqlalchemy#56

I don't know how we'd want to start showing extra stats from the job resource in the DB-API, but in this user's case just successfully executing with a dry run query is sufficient. They're updating their connection string to just try out the code for syntax errors before running it for real.

Loading

google/cloud/bigquery/dbapi/cursor.py Outdated Show resolved Hide resolved
Loading
@plamut plamut requested review from tswast and shollyman and removed request for shollyman Jun 12, 2020
Copy link
Contributor

@shollyman shollyman left a comment

Thanks for this!

Loading

@plamut plamut merged commit bc33a67 into googleapis:master Jun 23, 2020
3 checks passed
Loading
@plamut plamut deleted the iss-118 branch Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants