Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11888 )
Change subject: IMPALA-7795: Implement REFRESH AUTHORIZATION statement ...................................................................... Patch Set 11: (9 comments) http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java File fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java: http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@59 PS9, Line 59: REFRESH_TABLE(true), > We now have two flags: isRefresh, and authorization. Are these disjoint? On Yeah I agree this class is somewhat convoluted. I like your suggestion. Done. http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@111 PS9, Line 111: /*partition*/ null); > The logic here is getting hard to follow. Would be easier using a case stat Done http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@160 PS9, Line 160: } > Same comment as above. Done http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1140 PS9, Line 1140: public void refreshAuthorization(boolean resetVersions) throws CatalogException { > if (sentryProxy == null) return; Done http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1140 PS9, Line 1140: public void refreshAuthorization(boolean resetVersions) throws CatalogException { > if (sentryProxy == null) return; Done http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/jflex/sql-scanner.flex File fe/src/main/jflex/sql-scanner.flex: http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/main/jflex/sql-scanner.flex@78 PS9, Line 78: keywordMap.put("authorization", SqlParserSymbols.KW_AUTHORIZATION); > A persistent concern when adding keywords is that some queries will now fai I believe authorization is a reserved keyword as specified in L321 of the original code. REFRESH ROLE and REFRESH GRANT are too specific and not generic enough we want the statement to basically refresh anything related to authorization metadata. http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java: http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@331 PS9, Line 331: assertAction.accept(AnalyzesOk("invalidate metadata"), > Ideally, here or elsewhere, we'd inspect the resulting statement to ensure Good idea. Done. http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java: http://gerrit.cloudera.org:8080/#/c/11888/9/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@451 PS9, Line 451: * Creates an authorization config for creating an AnalysisContext with > Maybe explain this a bit: what is covered in the file? What does this enabl Done http://gerrit.cloudera.org:8080/#/c/11888/9/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: http://gerrit.cloudera.org:8080/#/c/11888/9/tests/authorization/test_authorization.py@547 PS9, Line 547: self.execute_query_expect_success(self.client, "refresh authorization") > Not specific to this test, but what do we return from this statement? Shoul Good idea. Done. -- To view, visit http://gerrit.cloudera.org:8080/11888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5459e1c97b12dee307e0cf85b94a9f66fd9d9a8c Gerrit-Change-Number: 11888 Gerrit-PatchSet: 11 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Thu, 06 Dec 2018 01:59:54 +0000 Gerrit-HasComments: Yes
