Uh oh!
There was an error while loading. Please reload this page.
feat: update execute_query to use PrepareQuery API - #1095
Conversation
de46f3f to
3279eb2Compare3279eb2 to
1474b01Compare
daniel-sanche
left a comment
There was a problem hiding this comment.
Looks good overall. I added a couple small comments, and a question around retries
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| Metadata will not be set until the first row has been yielded or response with no rows | ||
| completes. | ||
| """ | ||
| return self._final_metadata |
There was a problem hiding this comment.
returning None seems fine, but have you considered raising an exception if this is called in the wrong state?
I could imagine arguments for either one, depending on the use-case
There was a problem hiding this comment.
I dont have a strong opinion between those options. Do you have a preference? or is there anything else like this in the client we should be consistent with?
There was a problem hiding this comment.
I think I might have a slight preference towards an exception, because then we can attach a message explaining why it's empty. and it makes the types stronger, since we can avoid Optionals.
But I'm fine with either way, and it probably comes down to the context we expect people to use it in
(We have precedent for raising exceptions for something like this in firestore, but I can't think of any for bigtable)
There was a problem hiding this comment.
Sounds good. Updated it to raise an exception
Uh oh!
There was an error while loading. Please reload this page.
| result = await client.execute_query( | ||
| query, instance_id, parameters=parameters, parameter_types=param_types | ||
| ) |
There was a problem hiding this comment.
are there any new system tests that can be added? Maybe using the new prepare_* arguments? Or accessing iterator.metadata?
There was a problem hiding this comment.
I want to add at least one against a real table, and I want to refactor the Metadata to expose the fields in ProtoMetadata. I was planning both of those for a follow on if that's ok (still to the branch before we merge).
For prepare_* it's a bit tricky since those are all retry/timeout params
Uh oh!
There was an error while loading. Please reload this page.
0073253 to
441d71aCompare441d71a to
effaba2Compareeffaba2 to
8d47c13Compare
Update execute_query to use PrepareQuery for each request.
In there future there will also be an option to prepare once and reuse a PreparedStatement across requests. This updates the metadata to come from the PrepareResponse, but does so in a way that will be compatible with a future version that refreshes the PrepareResponse periodically (which can lead to schema change for e.g 'Select *' queries.
This also creates a sql_helpers test class and replaces old, more minimal, sql helpers in __testing
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:
Fixes #<issue_number_goes_here> 🦕