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.
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.
| @abc.abstractmethod | ||
| def _request_path(self) -> dict[str, str]: |
There was a problem hiding this comment.
The abstract method _request_path in the base class _DataApiTargetAsync is missing the @property decorator. Its subclasses (TableAsync, AuthorizedViewAsync, and MaterializedViewAsync) all implement _request_path as a property, and the synchronous counterpart _DataApiTarget._request_path is also defined as a property. Adding @property here ensures consistency, correct static analysis/type checking, and prevents potential runtime errors if the base class interface is called as a method.
@property
@abc.abstractmethod
def _request_path(self) -> dict[str, str]:| try: | ||
| test_fn = view.__getattribute__(fn_name) | ||
| maybe_stream = await test_fn(*fn_args) | ||
| [i async for i in maybe_stream] | ||
| except Exception: | ||
| # we expect an exception from attempting to call the mock | ||
| pass |
There was a problem hiding this comment.
In test_call_metadata, the test attempts to perform an asynchronous iteration ([i async for i in maybe_stream]) on the result of all read methods. However, read_row and row_exists are coroutines that return non-iterable objects (Row/None and bool respectively). This causes a TypeError which is silently swallowed by the broad except Exception: block. This can mask actual bugs in those methods. We should only perform the async iteration for methods that actually return an async iterable.
| try: | |
| test_fn = view.__getattribute__(fn_name) | |
| maybe_stream = await test_fn(*fn_args) | |
| [i async for i in maybe_stream] | |
| except Exception: | |
| # we expect an exception from attempting to call the mock | |
| pass | |
| try: | |
| test_fn = view.__getattribute__(fn_name) | |
| result = await test_fn(*fn_args) | |
| if fn_name not in {"read_row", "row_exists"}: | |
| [i async for i in result] | |
| except Exception: | |
| # we expect an exception from attempting to call the mock | |
| pass |
References
- In the google-cloud-bigtable Python async client, table.read_rows_stream() is a coroutine that returns an async iterable. It must be awaited before iterating, using the pattern async for row in await table.read_rows_stream(query):.
…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.
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