Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16976 )
Change subject: IMPALA-9234: Support Ranger row filtering policies ...................................................................... Patch Set 4: (8 comments) Thank Csaba's review! Addressed the comments. http://gerrit.cloudera.org:8080/#/c/16976/3/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/16976/3/be/src/common/global-flags.cc@344 PS3, Line 344: "increm > optional: I think that if we only use this in Java, then it is better to pl Good point. Move this and 'enable_column_masking' to backend-gflag-util.cc. I think lots of FE-only flags are defined here. We need to follow the new rule for future FE-only flags. http://gerrit.cloudera.org:8080/#/c/16976/3/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/16976/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@819 PS3, Line 819: uilder.grantOption(); : } > potential simplification: line 836 uses a very similar condition - can you Sure. I did some refactoring to make this resolveTableRef() function shorter. http://gerrit.cloudera.org:8080/#/c/16976/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@845 PS3, Line 845: Table sho > IMPALA-10482 uses the word "unrelative". I am not sure which one is better. Oops... Let's use 'non-relative' which is consistent to an existing comment here: https://github.com/apache/impala/blob/5baadd1da7d554ea3446e2a025afe8e991339765/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java#L498 http://gerrit.cloudera.org:8080/#/c/16976/3/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/16976/3/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@96 PS3, Line 96: needsRowFilter > I think that needsRowFiltering would be clearer. Done http://gerrit.cloudera.org:8080/#/c/16976/3/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/16976/3/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@421 PS3, Line 421: LOG.info("dbName: {}, tableName: {}, rowFilter: {}", dbName, tableName, filter); > Looks like temporary logging. It's intended. We have a similar log at line 402 for column masking. With them we can know which filter is choosed, which is helpful to understand the query results. Without them we can only explain the query and find the filters in the plan. Is it ok for you to keep them? http://gerrit.cloudera.org:8080/#/c/16976/3/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/16976/3/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationFactory.java@104 PS3, Line 104: supportsTableMasking > I don't understand why was the name changed - it returns isColumnMaskingEna Oops, we should also check row filtering as well. http://gerrit.cloudera.org:8080/#/c/16976/3/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/16976/3/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@527 PS3, Line 527: " }\n" + > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/16976/3/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@527 PS3, Line 527: " }\n" + > line too long (92 > 90) Done -- To view, visit http://gerrit.cloudera.org:8080/16976 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I580517be241225ca15e45686381b78890178d7cc Gerrit-Change-Number: 16976 Gerrit-PatchSet: 4 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: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 01 Mar 2021 03:41:51 +0000 Gerrit-HasComments: Yes
