Skip to content

fix(bigtable): add materialized view routing param to ReadRows and Sa… - #13918

Merged
mutianf merged 2 commits into
googleapis:mainfrom
mutianf:add-materialized-view-routing-params
Jul 28, 2026
Merged

fix(bigtable): add materialized view routing param to ReadRows and Sa…#13918
mutianf merged 2 commits into
googleapis:mainfrom
mutianf:add-materialized-view-routing-params

Conversation

@mutianf

@mutianf mutianf commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

…mpleRowKeys

ReadRows and SampleRowKeys can target a materialized view, but the client's request-params extractor only emitted table_name + app_profile_id and ignored materialized_view_name. Per the google.api.routing annotation in bigtable.proto, materialized views are instance-scoped and must route on the instance name.

Split the header composition into composeReadRequestParams (materialized-view capable) and composeWriteRequestParams (table/authorized-view only), and add NameUtil.extractInstanceNameFromMaterializedViewName.

Add HeadersTest coverage asserting the instance-name routing param for both operations, and a ReadIT integration test that reads through a materialized view.

@mutianf
mutianf requested review from a team as code owners July 28, 2026 01:31

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request adds support for routing read requests (ReadRows and SampleRowKeys) targeting materialized views in Cloud Bigtable. Since materialized views are instance-scoped, they route on the instance name instead of the table name. The changes introduce helper methods to extract the instance name, update the stub to handle this routing logic, and add corresponding tests. The feedback suggests omitting the app_profile_id parameter from the routing headers when it is empty to maintain consistency with write requests and avoid sending empty parameters to the server.

@mutianf
mutianf force-pushed the add-materialized-view-routing-params branch 2 times, most recently from f9be7f9 to f4c6c3d Compare July 28, 2026 01:44
mutianf added 2 commits July 28, 2026 02:11
…mpleRowKeys

ReadRows and SampleRowKeys can target a materialized view, but the client's
request-params extractor only emitted table_name + app_profile_id and ignored
materialized_view_name. Per the google.api.routing annotation in bigtable.proto,
materialized views are instance-scoped and must route on the instance name.

Split the header composition into composeReadRequestParams (materialized-view
capable) and composeWriteRequestParams (table/authorized-view only), and add
NameUtil.extractInstanceNameFromMaterializedViewName.

Add HeadersTest coverage asserting the instance-name routing param for both
operations, and a ReadIT integration test that reads through a materialized view.
@mutianf
mutianf force-pushed the add-materialized-view-routing-params branch from f4c6c3d to eda51af Compare July 28, 2026 02:13
@mutianf

mutianf commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces support for routing requests targeting materialized views in the Bigtable client. It updates EnhancedBigtableStub to extract instance-level routing parameters when a materialized view name is specified, and adds corresponding integration and header routing tests. The feedback suggests extracting the duplicated routing parameter extraction logic across the different callables in EnhancedBigtableStub into a single helper method to improve maintainability and code readability.

Comment on lines +378 to +385
r.getMaterializedViewName().isEmpty()
? composeRequestParams(
r.getAppProfileId(), r.getTableName(), r.getAuthorizedViewName())
: composeInstanceLevelRequestParams(
MaterializedViewName.parse(r.getMaterializedViewName())
.getInstanceName()
.toString(),
r.getAppProfileId()))

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.

medium

The logic to extract routing parameters for read operations (which can target a table, authorized view, or materialized view) is duplicated across multiple callables. Consider extracting this into a helper method to improve maintainability and readability.

Here is an example of the helper method you can add to the class:

private Map<String, String> composeReadRequestParams(
    String appProfileId,
    String tableName,
    String authorizedViewName,
    String materializedViewName) {
  if (materializedViewName.isEmpty()) {
    return composeRequestParams(appProfileId, tableName, authorizedViewName);
  }
  return composeInstanceLevelRequestParams(
      MaterializedViewName.parse(materializedViewName).getInstanceName().toString(),
      appProfileId);
}
                        composeReadRequestParams(
                            r.getAppProfileId(),
                            r.getTableName(),
                            r.getAuthorizedViewName(),
                            r.getMaterializedViewName())

Comment on lines +458 to +465
r.getMaterializedViewName().isEmpty()
? composeRequestParams(
r.getAppProfileId(), r.getTableName(), r.getAuthorizedViewName())
: composeInstanceLevelRequestParams(
MaterializedViewName.parse(r.getMaterializedViewName())
.getInstanceName()
.toString(),
r.getAppProfileId()))

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.

medium

This block duplicates the routing parameter extraction logic. Use the extracted composeReadRequestParams helper method here as well.

                        composeReadRequestParams(
                            r.getAppProfileId(),
                            r.getTableName(),
                            r.getAuthorizedViewName(),
                            r.getMaterializedViewName())

Comment on lines +614 to +623
r.getMaterializedViewName().isEmpty()
? composeRequestParams(
r.getAppProfileId(),
r.getTableName(),
r.getAuthorizedViewName())
: composeInstanceLevelRequestParams(
MaterializedViewName.parse(r.getMaterializedViewName())
.getInstanceName()
.toString(),
r.getAppProfileId()))

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.

medium

This block duplicates the routing parameter extraction logic. Use the extracted composeReadRequestParams helper method here as well.

                            composeReadRequestParams(
                                r.getAppProfileId(),
                                r.getTableName(),
                                r.getAuthorizedViewName(),
                                r.getMaterializedViewName())

@mutianf
mutianf merged commit 4ddf250 into googleapis:main Jul 28, 2026
217 of 219 checks passed
@mutianf
mutianf deleted the add-materialized-view-routing-params branch July 28, 2026 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants