feat(bigtable): support materialized views in the data client#17676
feat(bigtable): support materialized views in the data client#17676axyjo wants to merge 2 commits into
Conversation
Adds a MaterializedView class (async and sync) to the Bigtable data client, alongside Table and AuthorizedView, plus a client.get_materialized_view() factory. Materialized views are instance-scoped and read-only: read_rows and sample_row_keys route requests via materialized_view_name, while mutation methods raise NotImplementedError since the mutation RPCs cannot target a materialized view. To support targets without a backing table, table identity (table_id/table_name) moves from the shared _DataApiTarget base class into the Table and AuthorizedView subclasses.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for read-only Materialized Views in both synchronous and asynchronous Bigtable data clients, including the necessary client methods, documentation, and unit tests. Feedback highlights a missing @property decorator on the abstract _request_path method in _DataApiTargetAsync for consistency, and identifies a bug in the async unit tests where non-iterable return values from read_row and row_exists are incorrectly subjected to async iteration, which silently swallows errors.
…view metadata test read_row, row_exists, and the other non-streaming methods return concrete values; async-iterating them raised a TypeError that the surrounding except block silently swallowed, which could mask real errors.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for read-only materialized views in the Bigtable data client by adding the MaterializedView and MaterializedViewAsync classes, along with corresponding client methods, documentation, and unit tests. To accommodate this, the base classes _DataApiTarget and _DataApiTargetAsync were refactored to decouple table-specific attributes. Feedback on the changes identifies that the abstract method _request_path in _DataApiTargetAsync is missing the @property decorator, which should be added to ensure consistency with its subclasses and its synchronous counterpart.
| @abc.abstractmethod | ||
| def _request_path(self) -> dict[str, str]: |
There was a problem hiding this comment.
In _DataApiTargetAsync, the abstract method _request_path is missing the @property decorator. All of its subclasses (TableAsync, AuthorizedViewAsync, and MaterializedViewAsync) implement _request_path as a property, and the sync counterpart _DataApiTarget also defines it as a property. Adding @property here ensures consistency, prevents potential static analysis/type checking warnings, and matches the subclass implementations.
@property
@abc.abstractmethod
def _request_path(self) -> dict[str, str]:
mutianf
left a comment
mutianf
left a comment
There was a problem hiding this comment.
Lgtm but we should add a systest (in a follow up PR).
2 participants
Summary
Adds materialized view support to the Bigtable data client, alongside the existing
TableandAuthorizedViewsurfaces:MaterializedView/MaterializedViewAsyncclasses (subclasses of_DataApiTarget) and aclient.get_materialized_view(instance_id, materialized_view_id)factory. Reads (read_rows*,sample_row_keys,row_exists) route requests via thematerialized_view_namefield.materialized_view_namefield — somutate_row,bulk_mutate_rows,check_and_mutate_row,read_modify_write_row, andmutations_batcherare overridden to raiseNotImplementedErrorimmediately instead of failing with an opaque proto error (or, for the batcher, failing later in a background flush).table_id/table_name) moves from the shared_DataApiTargetbase class into theTableandAuthorizedViewsubclasses. Public constructor signatures are unchanged.nox -s generate_sync; docs pages added for both new classes.Test plan
TestMaterializedViewsuites (async + generated sync) cover construction, routing metadata (name=<instance path>header), factory passthrough, context-manager use, and the mutation-method rejection;get_materialized_viewadded to the existing API-surface parametrizationstests/unitsuite passes locally (5394 passed)ReadRows/SampleRowKeysrequests carrymaterialized_view_name(nottable_name), rows are returned through the public API, andTable/AuthorizedViewbehavior is unchanged