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 5: (6 comments) 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 'HdfsTable' is not used in LocalCatalog mode coordinators. Does this patch work in LocalCatalog mode? BTW, does MV support using Kudu as the storage? http://gerrit.cloudera.org:8080/#/c/17595/5/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/5/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@312 PS5, Line 312: if (!(tbl instanceof MaterializedViewHdfsTable)) { Could you add a comment for this? It seems MaterializedViewHdfsTable could not go here since it extends HdfsTable which extends CatalogObjectImpl whose isLoaded() always return true. http://gerrit.cloudera.org:8080/#/c/17595/5/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@380 PS5, Line 380: nit: blank line http://gerrit.cloudera.org:8080/#/c/17595/5/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/5/fe/src/main/java/org/apache/impala/catalog/MaterializedViewHdfsTable.java@75 PS5, Line 75: if (srcTbl == null) continue; Should we report errors on this case? http://gerrit.cloudera.org:8080/#/c/17595/5/fe/src/main/java/org/apache/impala/catalog/MaterializedViewHdfsTable.java@81 PS5, Line 81: return needsMaskingOrFiltering; I think we should not return here if needsMaskingOrFiltering is false. http://gerrit.cloudera.org:8080/#/c/17595/5/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/5/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@3110 PS5, Line 3110: onTable("functional", "materialized_view", TPrivilegeLevel.SELECT)); Could you add a more complex test that the number of srcTables of a MV is larger than one? -- 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: 5 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: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Fri, 18 Jun 2021 05:53:38 +0000 Gerrit-HasComments: Yes
