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 June 10, 2020 10:33
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 10, 2020
@shollyman
Copy link
Contributor

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?

@tswast
Copy link
Contributor

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.

@plamut plamut requested review from tswast and shollyman and removed request for shollyman June 12, 2020 14:35
Copy link
Contributor

@shollyman shollyman left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@plamut plamut merged commit bc33a67 into googleapis:master Jun 23, 2020
@plamut plamut deleted the iss-118 branch June 23, 2020 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dry run queries should not fail with 404:Not found job
4 participants