Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16976 )
Change subject: IMPALA-9234: Support Ranger row filtering policies ...................................................................... Patch Set 3: (6 comments) I went through the implementation but didn't look too much at the tests. Will do another pass soon. 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: DEFINE_bool optional: I think that if we only use this in Java, then it is better to place the define in backend-gflag-util.cc The reason why I did this for SAML related configs is that #include "kudu/util/flag_tags.h" caused link problems when it was included in global-flags.cc. Even if we don't use tagging, it makes more sense to me to keep only really global flags in this global-flags.cc 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: doTableMasking || !getAuthzFactory().getAuthorizationConfig().isEnabled() : || !getAuthzFactory().supportsTableMasking() potential simplification: line 836 uses a very similar condition - can you move it to a common bool variable? e.g. doTableMasking = doTableMasking && getAuthzFactory().getAuthorizationConfig().isEnabled() && getAuthzFactory().supportsTableMasking() http://gerrit.cloudera.org:8080/#/c/16976/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@845 PS3, Line 845: unrelated IMPALA-10482 uses the word "unrelative". I am not sure which one is better. 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: needsFiltering I think that needsRowFiltering would be clearer. 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. 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 isColumnMaskingEnabled(), which is specific to column masking -- 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: 3 Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 24 Feb 2021 14:41:56 +0000 Gerrit-HasComments: Yes
