Skip to content
This repository was archived by the owner on Apr 1, 2026. It is now read-only.

feat: async execute query client - #1011

Merged
daniel-sanche merged 10 commits into
googleapis:mainfrom
kboroszko:async-execute-query-client
Aug 8, 2024
Merged

feat: async execute query client#1011
daniel-sanche merged 10 commits into
googleapis:mainfrom
kboroszko:async-execute-query-client

Conversation

@kboroszko

Copy link
Copy Markdown
Contributor

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)

Description

This PR introduces support for the newly added executeQuery RPC in the Bigtable async client.

It handles executing parameterized queries and receiving streaming responses through an asynchronous iterator.
Most of the logic is located in google.cloud.bigtable.data.execute_query.
Example usage is in samples/snippets/data_client/data_client_snippets_async.py

@kboroszko
kboroszko requested a review from a team as a code ownerAugust 5, 2024 14:28
@kboroszko
kboroszko requested review from a team and msampathkumarAugust 5, 2024 14:28
@snippet-bot

snippet-botBot commented Aug 5, 2024

Copy link
Copy Markdown

Here is the summary of changes.

You are about to add 1 region tag.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@daniel-sanchedaniel-sanche left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Added a couple requests for missing docstrings, etc. But overall LGTM once the tests pass



class ParameterTypeInferenceFailed(ValueError):
pass

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: it would be good to have a docstring here

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

)


class ExecuteQueryIteratorAsync:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

docstring?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

def __aiter__(self):
return self

async def metadata(self) -> Optional[Metadata]:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please add docstrings on all of these methods

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

params: Optional[Dict[str, ExecuteQueryValueType]],
parameter_types: Optional[Dict[str, SqlType.Type]],
):
if not params:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

docstrings on these would also be helpful for future maintenance

It doesn't need to be too complex for the internal functions, but some context would be useful

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done



def _parse_array_type(value: PBValue, metadata_type: SqlType.Array) -> list:
return list(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

docstrings here too

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done.

Comment threadgoogle/cloud/bigtable_v2/__init__.py Outdated
"SampleRowKeysRequest",
"SampleRowKeysResponse",
"ExecuteQueryRequest",
"ExecuteQueryResponse",

@daniel-sanchedaniel-sancheAug 5, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This file is autogenerated and doesn't need to be touched (These are already included in lines 87/88)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

removed.

@@ -0,0 +1,149 @@
# Copyright 2024 Google LLC

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Previously, the test files mirrored the structure of the source files. It might make sense to throw these into an execute_query directory?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done.

@daniel-sanche

Copy link
Copy Markdown
Contributor

FYI, you can ignore the failing samples tests. There's a quota issue in the samples project we're looking in to

@product-auto-labelproduct-auto-labelBot added the api: bigtable Issues related to the googleapis/python-bigtable API. label Aug 6, 2024
@daniel-sanchedaniel-sanche added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 6, 2024
@yoshi-kokoroyoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 6, 2024
@daniel-sanchedaniel-sanche added the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 6, 2024
@gcf-owl-botgcf-owl-botBot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 6, 2024
@product-auto-labelproduct-auto-labelBot added the size: xl Pull request size is extra large. label Aug 7, 2024
)

def _to_value_pb_dict(self, value: Any) -> dict:
def _to_value_pb_dict(self, value: Any) -> Any:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's not too important, but do we have to use Any for these? Does dict[str, Any] not work?

@jackdingilianjackdingilian added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 8, 2024
@yoshi-kokoroyoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 8, 2024
Comment threadgoogle/cloud/bigtable/data/execute_query/values.py Outdated
query = (
"SELECT _key, os_build, connected_cell, connected_wifi "
f"from {table_id} WHERE _key=@row_key"
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know I said to ignore the samples tests, but it looks like there's actually an issue here:

google.api_core.exceptions.InvalidArgument: 400 Syntax error: Missing whitespace between literal and alias [at 1:87]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, there's a snippet lint issue that can be addressed using

cd samples/snippets/data_client
nox -s blacken

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To fix the invalid argument do: f"from `{table_id}` WHERE _key=@row_key" (wraps the table name in backticks)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Daniel pointed out the column names here aren't working either. All of those are qualifiers.

We should update the first line of the query to SELECT _key, stats_summary['os_build'], stats_summary['connected_cell'], stats_summary['connected_wifi']

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed.

@daniel-sanchedaniel-sanche left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

blocking until sample is addressed

@daniel-sanchedaniel-sanche added kokoro:run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Aug 8, 2024
@gcf-owl-botgcf-owl-botBot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 8, 2024
@yoshi-kokoroyoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 8, 2024
@daniel-sanchedaniel-sanche added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 8, 2024
@yoshi-kokoroyoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 8, 2024

@daniel-sanchedaniel-sanche left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@daniel-sanche
daniel-sanche merged commit 45bc8c4 into googleapis:mainAug 8, 2024
@release-pleaserelease-pleaseBot mentioned this pull request Aug 5, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: bigtableIssues related to the googleapis/python-bigtable API.size: xlPull request size is extra large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@kboroszko@daniel-sanche@jackdingilian@yoshi-kokoro@triplequark