Skip to content

Conversation

@zation99
Copy link
Contributor

@zation99 zation99 commented Mar 24, 2025

Description

The original approach will cause a "Unsupported table writer handle" error. We should use InsertHandle with the MaterializedViewHandle key.
The order also needs to have MaterializedViewHandle key before InsertHandle key, otherwise the insertHandle type will be replaced with MaterializedViewHandle.

Motivation and Context

Impact

Test Plan

refresh materialized view query

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== NO RELEASE NOTE ==

@zation99 zation99 requested a review from a team as a code owner March 24, 2025 01:10
@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Mar 24, 2025
@aditi-pandit aditi-pandit changed the title [fix]Fix MV mapping [native] Fix MV mapping Mar 24, 2025
@aditi-pandit
Copy link
Contributor

@zation99 : Is this the only change required for RefreshMaterializedView ? Does MaterializedViewHandle map to InsertTableHandle and then the rest of Velox code work as is for Materialized Views ? If yes, then would be great if you add an e2e test in https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/nativeworker as well.

Copy link
Contributor

@gggrace14 gggrace14 left a comment

Choose a reason for hiding this comment

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

Prestissimo. Nice!

@zation99 zation99 merged commit 21bdd48 into prestodb:master Mar 24, 2025
98 checks passed
@zation99
Copy link
Contributor Author

@zation99 : Is this the only change required for RefreshMaterializedView ? Does MaterializedViewHandle map to InsertTableHandle and then the rest of Velox code work as is for Materialized Views ? If yes, then would be great if you add an e2e test in https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/nativeworker as well.

Oh sorry I missed your comment. Yes that's all needed and Velox uses InsertHandle for this.
Let me add an e2e test in a follow-up PR.

@prestodb-ci prestodb-ci mentioned this pull request Mar 28, 2025
30 tasks
pradeepvaka pushed a commit to pradeepvaka/presto that referenced this pull request Apr 11, 2025
The original approach will cause a "Unsupported table writer handle" error. We should use InsertHandle with the MaterializedViewHandle key.
The order also needs to have MaterializedViewHandle key before InsertHandle key, otherwise the insertHandle type will be replaced with MaterializedViewHandle.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:Meta PR from Meta

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants