Skip to content

Conversation

@ceekay47
Copy link
Contributor

@ceekay47 ceekay47 commented Jan 30, 2026

Summary:
MV query optimizer fails to rewrite queries when the specified table name differs between the MV definition and the incoming query (ex: base_table vs schema.base_table).

This fix resolves table references to schema-qualified names, ensuring consistent table matching regardless of how the table was specified.

Reviewed By: zation99

Differential Revision: D91699496

Summary by Sourcery

Ensure materialized view query optimization consistently matches base tables regardless of schema qualification in table names.

Bug Fixes:

  • Fix materialized view rewrites failing when base tables are referenced with different schema qualifications between the MV definition and the incoming query.

Tests:

  • Add coverage to verify materialized view query optimization works when base tables are referenced both with and without schema-qualified names in various query shapes.

Summary:
MV query optimizer fails to rewrite queries when the specified table name differs between the MV definition and the incoming query (ex: `base_table` vs `schema.base_table`). 

This fix resolves table references to schema-qualified names, ensuring consistent table matching regardless of how the table was specified.

Reviewed By: zation99

Differential Revision: D91699496
@ceekay47 ceekay47 requested review from a team, feilong-liu and jaystarshot as code owners January 30, 2026 19:01
@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Jan 30, 2026
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Jan 30, 2026

Reviewer's Guide

Ensures the materialized view (MV) query optimizer matches base tables even when one side uses schema-qualified table names by normalizing table references, and adds tests covering various combinations of qualified and unqualified table names.

Sequence diagram for MV base table matching with normalized table names

sequenceDiagram
    actor OptimizerCaller
    participant MaterializedViewQueryOptimizer
    participant MaterializedViewInfo
    participant MaterializedViewUtils
    participant Metadata

    OptimizerCaller->>MaterializedViewQueryOptimizer: visitRelation(node, context)
    MaterializedViewQueryOptimizer->>MaterializedViewInfo: getBaseTable()
    MaterializedViewInfo-->>MaterializedViewQueryOptimizer: Optional baseTable

    alt baseTable is present
        MaterializedViewQueryOptimizer->>MaterializedViewUtils: resolveTableName(node, session, metadata)
        MaterializedViewUtils->>Metadata: createQualifiedObjectName(session, node, tableName)
        Metadata-->>MaterializedViewUtils: QualifiedObjectName
        MaterializedViewUtils-->>MaterializedViewQueryOptimizer: resolvedRelationFromQuery

        MaterializedViewQueryOptimizer->>MaterializedViewUtils: resolveTableName(baseTable, session, metadata)
        MaterializedViewUtils->>Metadata: createQualifiedObjectName(session, baseTable, tableName)
        Metadata-->>MaterializedViewUtils: QualifiedObjectName
        MaterializedViewUtils-->>MaterializedViewQueryOptimizer: resolvedRelationFromMV

        alt resolvedRelationFromQuery equals resolvedRelationFromMV
            MaterializedViewQueryOptimizer-->>OptimizerCaller: materializedView
        else mismatch
            MaterializedViewQueryOptimizer-->>OptimizerCaller: IllegalStateException
        end
    else baseTable not present
        MaterializedViewQueryOptimizer-->>OptimizerCaller: IllegalStateException
    end
Loading

Class diagram for updated MV table name resolution

classDiagram
    class MaterializedViewUtils {
        +Expression convertMaterializedDataPredicatesToExpression(Expression expression)
        +Expression convertSymbolReferencesToIdentifiers(Expression expression)
        +Relation resolveTableName(Relation relation, Session session, Metadata metadata)
    }

    class MaterializedViewQueryOptimizer {
        -Session session
        -Metadata metadata
        -MaterializedViewInfo materializedViewInfo
        -Relation materializedView
        +Node visitAliasedRelation(AliasedRelation node, Void context)
        +Node visitRelation(Relation node, Void context)
    }

    class Relation
    class Table {
        +QualifiedName getName()
    }
    class QualifiedObjectName {
        +String getSchemaName()
        +String getObjectName()
    }
    class QualifiedName {
        +static QualifiedName of(String schemaName, String objectName)
    }
    class Session
    class Metadata
    class Node
    class AliasedRelation
    class MaterializedViewInfo {
        +Optional getBaseTable()
    }

    MaterializedViewQueryOptimizer --> MaterializedViewUtils : uses_resolveTableName
    MaterializedViewUtils --> Relation
    MaterializedViewUtils --> Table
    MaterializedViewUtils --> QualifiedObjectName
    MaterializedViewUtils --> QualifiedName
    MaterializedViewUtils --> Session
    MaterializedViewUtils --> Metadata
    MaterializedViewQueryOptimizer --> Session
    MaterializedViewQueryOptimizer --> Metadata
    MaterializedViewQueryOptimizer --> MaterializedViewInfo
    MaterializedViewQueryOptimizer --> Node
    MaterializedViewQueryOptimizer --> AliasedRelation
    Table --|> Relation
Loading

File-Level Changes

Change Details Files
Normalize table relations to schema-qualified names before comparing MV base tables during query rewrite.
  • Introduce resolveTableName helper that resolves a Relation to a schema-qualified Table using createQualifiedObjectName and Metadata
  • Return the original relation unchanged when it is not a Table instance
  • Construct a new Table node using the resolved schema and table name only (catalog omitted)
presto-main-base/src/main/java/com/facebook/presto/sql/MaterializedViewUtils.java
Use normalized table names when deciding whether to rewrite a query to use a materialized view and add regression tests for schema-qualified names.
  • Update MaterializedViewQueryOptimizer to compare base tables using resolveTableName for both the query relation and the MV base table
  • Throw the same IllegalStateException for mismatching tables, preserving existing error behavior aside from name normalization
  • Add testWithSchemaQualifiedTableName to cover all combinations of qualified/unqualified base table names in MV definition and queries, including projection, filter, and aggregation scenarios
presto-main-base/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
presto-main-base/src/test/java/com/facebook/presto/sql/analyzer/TestMaterializedViewQueryOptimizer.java

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 1 issue, and left some high level feedback:

  • In resolveTableName, you strip the catalog and always return a Table with only schema.table; if the original relation was catalog-qualified, this could break matching for multi-catalog setups—consider preserving the catalog component in the returned QualifiedName.
  • The resolveTableName helper only handles Table instances; since visitRelation is called on a generic Relation, it may be safer to either enforce/validate that only Table nodes reach this code path or to handle/warn on other Relation subtypes explicitly to avoid surprising behavior.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `resolveTableName`, you strip the catalog and always return a `Table` with only `schema.table`; if the original relation was catalog-qualified, this could break matching for multi-catalog setups—consider preserving the catalog component in the returned `QualifiedName`.
- The `resolveTableName` helper only handles `Table` instances; since `visitRelation` is called on a generic `Relation`, it may be safer to either enforce/validate that only `Table` nodes reach this code path or to handle/warn on other `Relation` subtypes explicitly to avoid surprising behavior.

## Individual Comments

### Comment 1
<location> `presto-main-base/src/test/java/com/facebook/presto/sql/analyzer/TestMaterializedViewQueryOptimizer.java:453-454` </location>
<code_context>
         assertOptimizedQuery(baseQuerySqlWithTablePrefix, expectedRewrittenSql, originalViewSqlWithTablePrefix, BASE_TABLE_1, VIEW_1);
     }

+    @Test
+    public void testWithSchemaQualifiedTableName()
+    {
+        String schemaQualifiedTable = SESSION_SCHEMA + "." + BASE_TABLE_1;
</code_context>

<issue_to_address>
**suggestion (testing):** Add negative tests for mismatching schemas/table names to prove we don’t rewrite when we shouldn’t

To make the optimizer behavior more robustly tested, please add one or two negative cases where the schema-qualified name resolves to a *different* table than the MV base table (e.g., `SESSION_SCHEMA.other_table` or `other_schema.base_table`). These should assert that the MV rewrite does not occur (or that the correct exception path is taken), ensuring the new `resolveTableName` logic does not over-match tables.

Suggested implementation:

```java
    @Test
    public void testWithSchemaQualifiedTableName()
    {
        String schemaQualifiedTable = SESSION_SCHEMA + "." + BASE_TABLE_1;

        String originalViewSql = format("SELECT a, b FROM %s", BASE_TABLE_1);
        String baseQuerySql = format("SELECT a, b FROM %s", schemaQualifiedTable);
        String expectedRewrittenSql = format("SELECT a, b FROM %s", VIEW_1);

        assertOptimizedQuery(baseQuerySql, expectedRewrittenSql, originalViewSql, BASE_TABLE_1, VIEW_1);

        originalViewSql = format("SELECT a, b FROM %s", schemaQualifiedTable);
        baseQuerySql = format("SELECT a, b FROM %s", BASE_TABLE_1);
        expectedRewrittenSql = format("SELECT a, b FROM %s", VIEW_1);

        assertOptimizedQuery(baseQuerySql, expectedRewrittenSql, originalViewSql, BASE_TABLE_1, VIEW_1);
    }

    @Test
    public void testWithSchemaQualifiedTableNameMismatchingTable()
    {
        // MV is defined over BASE_TABLE_1; query uses a different table in the same schema
        String schemaQualifiedOtherTable = SESSION_SCHEMA + "." + BASE_TABLE_2;

        String originalViewSql = format("SELECT a, b FROM %s", BASE_TABLE_1);
        String baseQuerySql = format("SELECT a, b FROM %s", schemaQualifiedOtherTable);

        // Verify that a different table name in the same schema does NOT trigger a rewrite
        assertNotOptimizedQuery(baseQuerySql, originalViewSql, BASE_TABLE_1, VIEW_1);
    }

    @Test
    public void testWithSchemaQualifiedTableNameMismatchingSchema()
    {
        // MV is defined over SESSION_SCHEMA.BASE_TABLE_1; query uses the same table name in a different schema
        String otherSchemaQualifiedTable = "other_schema." + BASE_TABLE_1;

        String originalViewSql = format("SELECT a, b FROM %s", SESSION_SCHEMA + "." + BASE_TABLE_1);
        String baseQuerySql = format("SELECT a, b FROM %s", otherSchemaQualifiedTable);

        // Verify that a different schema for the same table name does NOT trigger a rewrite
        assertNotOptimizedQuery(baseQuerySql, originalViewSql, BASE_TABLE_1, VIEW_1);

```

To fully wire these tests into the existing test harness, you will also need to:
1. Ensure there is a `BASE_TABLE_2` constant defined in this test class (or adjust `testWithSchemaQualifiedTableNameMismatchingTable` to use whatever second base-table constant already exists).
2. Implement or reuse an assertion helper like `assertNotOptimizedQuery(String baseQuerySql, String originalViewSql, String baseTable, String viewName)` that:
   - Runs the materialized view optimizer on `baseQuerySql` given the provided MV definition (`originalViewSql`, `baseTable`, `viewName`).
   - Asserts that either:
     a) The optimized query text is identical to `baseQuerySql` (no rewrite occurred), or  
     b) The optimizer throws the expected exception for non-rewritable queries (if that is the intended behavior).
3. If such a helper already exists under a different name/signature (e.g., `assertNoRewrite`, `assertNotRewrittenQuery`, etc.), replace `assertNotOptimizedQuery(...)` calls with the appropriate existing helper and adjust the arguments accordingly.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +453 to +454
@Test
public void testWithSchemaQualifiedTableName()
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (testing): Add negative tests for mismatching schemas/table names to prove we don’t rewrite when we shouldn’t

To make the optimizer behavior more robustly tested, please add one or two negative cases where the schema-qualified name resolves to a different table than the MV base table (e.g., SESSION_SCHEMA.other_table or other_schema.base_table). These should assert that the MV rewrite does not occur (or that the correct exception path is taken), ensuring the new resolveTableName logic does not over-match tables.

Suggested implementation:

    @Test
    public void testWithSchemaQualifiedTableName()
    {
        String schemaQualifiedTable = SESSION_SCHEMA + "." + BASE_TABLE_1;

        String originalViewSql = format("SELECT a, b FROM %s", BASE_TABLE_1);
        String baseQuerySql = format("SELECT a, b FROM %s", schemaQualifiedTable);
        String expectedRewrittenSql = format("SELECT a, b FROM %s", VIEW_1);

        assertOptimizedQuery(baseQuerySql, expectedRewrittenSql, originalViewSql, BASE_TABLE_1, VIEW_1);

        originalViewSql = format("SELECT a, b FROM %s", schemaQualifiedTable);
        baseQuerySql = format("SELECT a, b FROM %s", BASE_TABLE_1);
        expectedRewrittenSql = format("SELECT a, b FROM %s", VIEW_1);

        assertOptimizedQuery(baseQuerySql, expectedRewrittenSql, originalViewSql, BASE_TABLE_1, VIEW_1);
    }

    @Test
    public void testWithSchemaQualifiedTableNameMismatchingTable()
    {
        // MV is defined over BASE_TABLE_1; query uses a different table in the same schema
        String schemaQualifiedOtherTable = SESSION_SCHEMA + "." + BASE_TABLE_2;

        String originalViewSql = format("SELECT a, b FROM %s", BASE_TABLE_1);
        String baseQuerySql = format("SELECT a, b FROM %s", schemaQualifiedOtherTable);

        // Verify that a different table name in the same schema does NOT trigger a rewrite
        assertNotOptimizedQuery(baseQuerySql, originalViewSql, BASE_TABLE_1, VIEW_1);
    }

    @Test
    public void testWithSchemaQualifiedTableNameMismatchingSchema()
    {
        // MV is defined over SESSION_SCHEMA.BASE_TABLE_1; query uses the same table name in a different schema
        String otherSchemaQualifiedTable = "other_schema." + BASE_TABLE_1;

        String originalViewSql = format("SELECT a, b FROM %s", SESSION_SCHEMA + "." + BASE_TABLE_1);
        String baseQuerySql = format("SELECT a, b FROM %s", otherSchemaQualifiedTable);

        // Verify that a different schema for the same table name does NOT trigger a rewrite
        assertNotOptimizedQuery(baseQuerySql, originalViewSql, BASE_TABLE_1, VIEW_1);

To fully wire these tests into the existing test harness, you will also need to:

  1. Ensure there is a BASE_TABLE_2 constant defined in this test class (or adjust testWithSchemaQualifiedTableNameMismatchingTable to use whatever second base-table constant already exists).
  2. Implement or reuse an assertion helper like assertNotOptimizedQuery(String baseQuerySql, String originalViewSql, String baseTable, String viewName) that:
    • Runs the materialized view optimizer on baseQuerySql given the provided MV definition (originalViewSql, baseTable, viewName).
    • Asserts that either:
      a) The optimized query text is identical to baseQuerySql (no rewrite occurred), or
      b) The optimizer throws the expected exception for non-rewritable queries (if that is the intended behavior).
  3. If such a helper already exists under a different name/signature (e.g., assertNoRewrite, assertNotRewrittenQuery, etc.), replace assertNotOptimizedQuery(...) calls with the appropriate existing helper and adjust the arguments accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants