feat(connector): Add warning log for MV read permission fallback

[cjac]

feat(connector): Add warning log for MV read permission fallback

When reading a Materialized View without 'bigquery.tables.getData'
permission, the connector silently falls back to re-executing the view's
query. This is opaque to the user and causes unexpected costs and poor
performance, which are difficult to diagnose.

This change introduces a try-catch block to detect a permission-denied
error (HTTP 403) when attempting a direct read of a Materialized View.
If this specific error is caught, a detailed WARN message is logged,
explaining the cause of the fallback and instructing the user how to
resolve it by granting the 'roles/bigquery.dataViewer' role.

A new integration test, MaterializedViewReadIT, has been added to
verify this behavior by impersonating a service account with
insufficient permissions and asserting that the warning is logged.

This significantly improves the supportability and user experience of the
connector by making the fallback behavior transparent and empowering
users to self-resolve the underlying permissions issue.

Related to: https://issuetracker.google.com/296281345
Related to: dataform-co/dataform#1640

Resolves #1401

[gemini-code-assist]

Summary of Changes

Hello @cjac, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the user experience and supportability of the BigQuery connector when dealing with Materialized Views. It introduces explicit logging for a common permission-related fallback scenario, making the connector's behavior more transparent and providing users with clear guidance to optimize their BigQuery interactions.

Highlights

• Improved Materialized View Read Experience: I've added a mechanism to detect when a direct read from a BigQuery Materialized View fails due to insufficient permissions (bigquery.tables.getData). Instead of silently falling back to re-executing the view's query (which is costly and opaque), the connector now logs a detailed WARN message.
• Actionable Warning Message: The new warning log clearly explains the permission issue, the fallback behavior, and provides instructions on how to resolve it by granting the roles/bigquery.dataViewer role, empowering users to self-diagnose and fix performance/cost issues.
• Refactoring for Clarity: The createReadSession logic within ReadSessionCreator.java has been extracted into a new private helper method to improve code organization and readability, especially around the new Materialized View handling.
• Comprehensive Integration Test: A new integration test, MaterializedViewReadIT, has been added. This test specifically verifies the new warning log and fallback behavior by impersonating a service account with deliberately insufficient permissions for direct MV reads, ensuring the feature works as intended.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a helpful warning when a direct read on a Materialized View fails due to permissions, and falls back to materializing the view. However, a critical issue in the fallback mechanism seems to prevent it from working as intended. There are also a couple of medium-severity suggestions to improve code clarity and maintainability.

[cjac]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a valuable improvement by adding a warning when a read on a Materialized View falls back to query execution due to missing permissions. This enhances user experience and supportability. The review includes suggestions to improve code clarity, consistency, and test robustness.

[cjac]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a valuable improvement by adding a warning when the connector falls back to re-executing a Materialized View's query due to permission issues. This enhances transparency and helps users diagnose performance problems and unexpected costs. The new integration test is well-written and effectively validates the new behavior.

I've identified one critical issue that will prevent compilation and one medium-severity issue in the new test file to improve its robustness.

[cjac]

/gemini review

[gemini-code-assist]

Code Review

The code changes introduce a warning log for Materialized View read permission fallback, enhancing user experience and supportability. The new integration test validates the new behavior.

[cjac]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a valuable improvement by adding a warning when a read on a Materialized View falls back to re-executing the query due to permission issues. This makes a previously silent and potentially costly behavior transparent to the user. The implementation with a try-catch block is straightforward, and the new integration test effectively validates the new functionality. My review includes a couple of suggestions to make the error handling more robust and to improve the clarity of the new log message.

[cjac]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a valuable improvement by adding a warning when a read on a Materialized View falls back to query execution due to insufficient permissions. This makes a previously silent and potentially costly behavior transparent to the user. The implementation is solid, with a new try-catch block to detect the specific permission error and a comprehensive integration test to verify the new logging behavior.

[cjac]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a valuable improvement by adding a warning when a read from a Materialized View falls back to re-executing the query due to permission issues, enhancing user experience and supportability. The implementation is clean and includes a new private method to reduce code duplication. The accompanying integration test is thorough in its setup to correctly simulate the permission-denied scenario.

My review identifies one critical issue in the new integration test: a potential race condition that could lead to test flakiness. I've provided a suggestion to make the test robust by ensuring it waits for the materialized view creation to complete.

[cjac]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a valuable improvement by adding a warning when the connector falls back to re-executing a Materialized View's query due to permission issues. This makes a previously silent and potentially costly behavior transparent to the user. The implementation is clean, with the core logic encapsulated in a try-catch block for Materialized Views. The addition of the MaterializedViewReadIT integration test is excellent, as it thoroughly validates the new behavior under the intended scenario. I have one minor suggestion to improve log message consistency.

[cjac]

/gemini review

[gemini-code-assist]

Code Review

The code changes introduce a warning log for Materialized View read permission fallback. The new warning is clear and helpful, and the addition of the MaterializedViewReadIT integration test is excellent. There's a critical issue related to filter and snapshot-time pushdown for direct materialized view reads that would cause filters to be silently ignored. Please address this.

[cjac]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a valuable feature by adding a warning when the connector falls back to materializing a Materialized View due to permission errors. This greatly improves user experience and diagnosability. The new integration test is also well-written and covers the new behavior effectively.

However, the refactoring has introduced a critical bug that causes filters to be applied twice when reading from any type of view (VIEW or MATERIALIZED_VIEW). I've left detailed comments on how to fix this issue, which involves adjusting the signature of the new createReadSession method and correcting the logic that decides whether to apply a row-level filter. Please address this issue before merging.

[cjac]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a valuable warning message when the connector falls back to re-executing a materialized view's query due to permission issues. The implementation is clean, and the new integration test effectively validates the behavior. A suggestion has been made to improve the resource cleanup logic in the new test to make it more robust.

[cjac]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a valuable feature by adding a warning log when a read from a Materialized View falls back to re-executing the view's query due to permission issues. This is a great improvement for user experience and supportability, as it makes a previously silent and potentially costly behavior transparent.

The implementation in ReadSessionCreator is well-targeted, using a try-catch block to detect the specific permission error. The new integration test, MaterializedViewReadIT, is comprehensive and effectively validates the new functionality by simulating the exact scenario with a dedicated service account.

My main feedback is a suggestion to refactor the permission-checking logic to enhance readability and maintainability by extracting it into a helper method and removing a magic string. Overall, this is a solid contribution.

[cjac]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a valuable warning when a materialized view read falls back to a more expensive query execution due to permission issues. The implementation is robust, using a try-catch block to detect the specific error and logging a helpful message to guide the user. The new integration test is well-designed, effectively verifying the fallback behavior and the new warning. Overall, this is a high-quality contribution that significantly improves the connector's usability and supportability.

[cjac]

/gcbrun

[cjac]

/gcbrun

[cjac]

/gcbrun

[cjac]

/gcbrun

[cjac]

/gcbrun