Aman Sinha 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 11: (9 comments) Thanks Quanlong and Qifan for the reviews. This patch had gone on my backburner for a while due to other priorities. I have addressed your review comments in the latest patchset. Pls take another look. 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 > Hive can create Kudu external tables: https://cwiki.apache.org/confluence/d Yes, for now this patch ignores Kudu. Regarding local catalog mode, I verified that the core functionality of querying and compute stats of the MV works when impala is started in the local_catalog mode. The reason is that on the catalogd side, the MaterializedViewHdfsTable instance is created (derived class of HdfsTable) but on the impalad side, LocalTable.loadTableMetadata() is called which receives a thrift response from the catalogd and creates the relevant LocalFsTable object. I have not checked the ranger authorization part in the local catalog mode but will do that in a follow-up jira if that's ok. http://gerrit.cloudera.org:8080/#/c/17595/7/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: http://gerrit.cloudera.org:8080/#/c/17595/7/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@464 PS7, Line 464: table_ instanceof MaterializedViewHdfsTable > I wonder similar check should be done in ModifyStmt.java. (https://github.c Impala only allows updates/deletes on Kudu tables, so if you do an UPDATE on an MV, it will actually fall through to the following line: https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java#L164 Which prints "Impala does not support modifying a non-Kudu table" This is true for regular tables and MVs just inherit that behavior. 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.isLoaded()) { > Could you add a comment for this? It seems MaterializedViewHdfsTable could Yes, after further analysis, these 2 lines are not needed anymore, so I have removed them. I think there was an intermediate phase of the patch where I needed this since the first invocation of this function would encounter an IncompleteTable object but subsequently the concrete table object is created. http://gerrit.cloudera.org:8080/#/c/17595/5/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@380 PS5, Line 380: return tableNames; > nit: blank line Done 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: TableMask tableMask = new Tab > Should we report errors on this case? I left it there as I wasn't sure if it could be a valid state . I will do some follow up to see if adding a preconditions check is ok. http://gerrit.cloudera.org:8080/#/c/17595/5/fe/src/main/java/org/apache/impala/catalog/MaterializedViewHdfsTable.java@81 PS5, Line 81: > I think we should not return here if needsMaskingOrFiltering is false. Good point. Corrected it. http://gerrit.cloudera.org:8080/#/c/17595/7/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/7/fe/src/main/java/org/apache/impala/catalog/MaterializedViewHdfsTable.java@70 PS7, Line 70: (db); > nit. The test implies that there must be at least one table name added to I was being paranoid initially but you're right.. it can be removed. Made this change. 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: onServer(TPrivilegeLevel.ALL)); > Yes, I was in fact thinking of expanding the scope of the tests since previ Added a new MV called mv1_alltypes_jointbl which has a join of 2 new source tables: alltypes_transactional and jointbl_transactional. I needed to create a transactional version of these tables because MVs can only be defined on transactional source tables. In this authorization test I have created a column masking policy on one of the source tables and querying the MV should fail. http://gerrit.cloudera.org:8080/#/c/17595/7/testdata/workloads/functional-query/queries/QueryTest/compute-stats.test File testdata/workloads/functional-query/queries/QueryTest/compute-stats.test: http://gerrit.cloudera.org:8080/#/c/17595/7/testdata/workloads/functional-query/queries/QueryTest/compute-stats.test@1841 PS7, Line 1841: table > nit. May allow keyword MV here too. You mean 'show materialized view stats .. ' ? Since this is now treated as a table, I think it is ok to not add a new keyword to the syntax here. -- 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: 11 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: Wed, 23 Mar 2022 03:24:16 +0000 Gerrit-HasComments: Yes
