Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17595 )
Change subject: IMPALA-10723: Treat materialized view as a table instead of a view ...................................................................... Patch Set 12: (6 comments) > Patch Set 12: > > > Patch Set 12: Verified-1 > > > > Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7961/ > > A few of the new tests I added for this patch failed due to the materialized > view table being empty. On my local machine it works fine. I looked at the > logs from the jenkins run and see that the CREATE MATERIALIZED VIEW .. FROM > alltypes_transactional join jointbl_transactional ... is being executed even > before the INSERT commands into those source tables are executed, so the MV > remains empty. I will check if this can be addressed through a > DEPENDENT_LOAD_HIVE section. Not sure if this is applicable for materialized > views. I think we can refresh the MV in the DEPENDENT_LOAD_HIVE section: ALTER MATERIALIZED VIEW [db_name.]materialized_view_name REBUILD; https://cwiki.apache.org/confluence/display/Hive/Materialized+views#Materializedviews-Materializedviewmaintenance BTW, really happy to see this work getting revived. Thanks for working on it! http://gerrit.cloudera.org:8080/#/c/17595/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17595/5//COMMIT_MSG@18 PS5, Line 18: makes the MVs a derived class of HdfsTable > Yes, for now this patch ignores Kudu. Sure. I think the GVO job will verifiy the ranger authorization part in the local catalog mode. Let's see if it catches any issues. http://gerrit.cloudera.org:8080/#/c/17595/11/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/17595/11/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@225 PS11, Line 225: mvAuthException > added the suffix. It seems the suffix is not added yet. BTW, I think we can remove the boolean variable and replace it with a check on (mvAuthExceptionMsg != null). http://gerrit.cloudera.org:8080/#/c/17595/11/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@931 PS11, Line 931: } else if (tableMask.needsMaskingOrFiltering()) { If the MV has masking policies, we won't go to the below check. Should we move the added check before this? http://gerrit.cloudera.org:8080/#/c/17595/11/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java File fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/17595/11/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@322 PS11, Line 322: ((MaterializedViewHdfsTable) tbl).addSrcTables(mvSrcTableNames); Can we define MV on some real views? In this case, 'mvSrcTableNames' could be a view name. addSrcTables() doens't add all the source tables. Note that collectTableCandidates() can only collect the table names. They are not resolved until we go into this method recursively again (at line 337). http://gerrit.cloudera.org:8080/#/c/17595/11/fe/src/main/java/org/apache/impala/catalog/MaterializedViewHdfsTable.java File fe/src/main/java/org/apache/impala/catalog/MaterializedViewHdfsTable.java: http://gerrit.cloudera.org:8080/#/c/17595/11/fe/src/main/java/org/apache/impala/catalog/MaterializedViewHdfsTable.java@84 PS11, Line 84: } nit: need a blank line after this http://gerrit.cloudera.org:8080/#/c/17595/11/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/17595/11/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@3171 PS11, Line 3171: Can we add an additional test that also add a masking policies on the MV? IIUC, we should see the same error. Just want to add the test coverage for the question I commented in Analyzer#resolveTableMask(). -- To view, visit http://gerrit.cloudera.org:8080/17595 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If3108996124c6544a97fb0c34b6aff5e324a6cff Gerrit-Change-Number: 17595 Gerrit-PatchSet: 12 Gerrit-Owner: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Thu, 24 Mar 2022 08:54:27 +0000 Gerrit-HasComments: Yes
