Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14894 )
Change subject: IMPALA-9009: Core support for Ranger column masking ...................................................................... Patch Set 7: (11 comments) Some initial comments, I still didn't digest thing 100%. http://gerrit.cloudera.org:8080/#/c/14894/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14894/7//COMMIT_MSG@10 PS7, Line 10: to specific users when reading specific columns. This patch add support nit: adds http://gerrit.cloudera.org:8080/#/c/14894/7/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/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@784 PS7, Line 784: return new CollectionTableRef(tableRef, resolvedPath); Can we mask values inside collections? http://gerrit.cloudera.org:8080/#/c/14894/6/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java File fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java: http://gerrit.cloudera.org:8080/#/c/14894/6/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java@153 PS6, Line 153: in typo: if? http://gerrit.cloudera.org:8080/#/c/14894/7/fe/src/main/java/org/apache/impala/authorization/TableMask.java File fe/src/main/java/org/apache/impala/authorization/TableMask.java: http://gerrit.cloudera.org:8080/#/c/14894/7/fe/src/main/java/org/apache/impala/authorization/TableMask.java@67 PS7, Line 67: getColumnMask nit, naming: I think that this function does too many things for a "get" function, I would rename it to createColumnMask or something similar http://gerrit.cloudera.org:8080/#/c/14894/7/fe/src/main/java/org/apache/impala/authorization/TableMask.java@69 PS7, Line 69: getColumnMask same as line 67 http://gerrit.cloudera.org:8080/#/c/14894/7/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/14894/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@278 PS7, Line 278: result nit, naming: result suggests to me that the function will return this variable http://gerrit.cloudera.org:8080/#/c/14894/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@297 PS7, Line 297: else if Is it ok to just pass through and return the original column? Shouldn't we return in line 280 in that case? http://gerrit.cloudera.org:8080/#/c/14894/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@298 PS7, Line 298: maskedColumn This branch seems a bit weird to me - what is the difference between a custom mask and a transformer? http://gerrit.cloudera.org:8080/#/c/14894/7/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/14894/7/tests/authorization/test_ranger.py@605 PS7, Line 605: "{0}/service/public/v2/api/policy?servicename=test_impala&policyname={1}".format( nit, here and at many other places: please try to use +4 indentation for broken lines http://gerrit.cloudera.org:8080/#/c/14894/7/tests/authorization/test_ranger.py@796 PS7, Line 796: don nit: doesn't http://gerrit.cloudera.org:8080/#/c/14894/7/tests/authorization/test_ranger.py@832 PS7, Line 832: self.try_execute_query(admin_client, "drop table if exists functional.masked_tbl") : self.try_execute_query(admin_client, "drop view if exists functional.masked_view") I would prefer to create a unique database for these to avoid the possibility of making functional "dirty". -- 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: 7 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: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Wed, 18 Dec 2019 16:33:47 +0000 Gerrit-HasComments: Yes
