feat: update execute_query to use PrepareQuery API #1095
Conversation
de46f3f to
3279eb2
Compare
3279eb2 to
1474b01
Compare
google/cloud/bigtable/data/execute_query/_async/execute_query_iterator.py
Show resolved
Hide resolved
| 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
google/cloud/bigtable/data/execute_query/_async/execute_query_iterator.py
Show resolved
Hide resolved
| @@ -1116,6 +1122,7 @@ async def test_execute_query_params(self, client, table_id, instance_id): | |||
| "tsArrayParam": SqlType.Array(SqlType.Timestamp()), | |||
| "dateArrayParam": SqlType.Array(SqlType.Date()), | |||
| } | |||
|
|
|||
| 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
0073253 to
441d71a
Compare
441d71a to
effaba2
Compare
effaba2 to
8d47c13
Compare
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> 🦕