Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14894 )
Change subject: IMPALA-9009: Core support for Ranger column masking ...................................................................... Patch Set 17: (4 comments) > Mostly concerned if adding the inline view is necessary and whether that > precludes other optimizations. I'm agree that we can support column masking using expression rewrite. But adding the table masking view has smaller modification on the AST. The table masking view also makes it easy to support row filtering policies in the future. We just need to add row filtering expressions in the WHERE clause of the inline view. For optimizations, I think it's fine. We have logics about pushing predicates down into the view (e.g. SingleNodePlanner#migrateConjunctsToInlineView). I think it's the duty of the optimizer to consider query performance. In this part we just need to make sure the result is correct. BTW, if we find any edge cases, it's also an opportunity to improve our optimizer. http://gerrit.cloudera.org:8080/#/c/14894/16/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/14894/16/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@760 PS16, Line 760: resolvedTableRef = new InlineViewRef((FeView) table, tableRef); > Does adding this view boundary preclude any subsequent optimizations such a Maybe you mean line-779 that introduces the table masking view? This line does not add a view. TableRef is an AST node. It could refer to a table or view. This line means if its path resolved to be a name of a view, we resolve it to be an InlineViewRef. For your question on line-779, I think it's fine. We have logics about migrating predicates into the view (e.g. SingleNodePlanner#migrateConjunctsToInlineView). I think it's the duty of the optimizer to consider query performance. In this part we just need to make sure the result is correct. http://gerrit.cloudera.org:8080/#/c/14894/16/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@771 PS16, Line 771: return resolvedTableRef; > Should BaseTableRef be returned in this case as before? No, resolvedTableRef here is the resolved TableRef before masking. It could be BaseTableRef or InlineViewRef. http://gerrit.cloudera.org:8080/#/c/14894/16/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationFactory.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationFactory.java: http://gerrit.cloudera.org:8080/#/c/14894/16/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationFactory.java@104 PS16, Line 104: public boolean supportsColumnMasking() { > We still don't support row filtering.. Done http://gerrit.cloudera.org:8080/#/c/14894/16/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java File fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java: http://gerrit.cloudera.org:8080/#/c/14894/16/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@249 PS16, Line 249: authzChecker_ = new RangerAuthorizationCheckerSpy(authzConfig_); > Any synchronizationb needed here? I think there are no race conditions here since the only two tests (testAuditLogSuccess and testAuditLogFailure) are ran one by one. -- To view, visit http://gerrit.cloudera.org:8080/14894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4cad60e0e69ea573b7ecfc011b142c46ef52ed61 Gerrit-Change-Number: 14894 Gerrit-PatchSet: 17 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Tue, 14 Jan 2020 08:02:54 +0000 Gerrit-HasComments: Yes
