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 9: (11 comments) Thanks for your feedbacks, Csaba! Added more comments about the implementation. Refactored the tests. 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 adds support > nit: adds Done 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? Hive does not support referencing nested types as tables. E.g. this query is illegal in Hive: select * from functional_parquet.complextypestbl.int_map; I think we can support masking primitive sub-types in complex types later. It can be an advance feature. Filed IMPALA-9263 for this. 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: iz > typo: if? Actually want to mean "in use" or materialized columns 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: createColumnM > nit, naming: I think that this function does too many things for a "get" fu good point http://gerrit.cloudera.org:8080/#/c/14894/7/fe/src/main/java/org/apache/impala/authorization/TableMask.java@69 PS7, Line 69: createColumnM > same as line 67 Done 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: ) thro > nit, naming: result suggests to me that the function will return this varia Done http://gerrit.cloudera.org:8080/#/c/14894/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@297 PS7, Line 297: transfo > Is it ok to just pass through and return the original column? Shouldn't we No. Line 280 means this column don't have column masking policy. Here we need to replace the original column based on the transfomer. I added more comments about tranformer. It's a concept in Ranger. http://gerrit.cloudera.org:8080/#/c/14894/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@298 PS7, Line 298: > This branch seems a bit weird to me - what is the difference between a cust Sorry for being unclear. Added more comments. Transfomer is an expression about how to tranform the target column. It references the column by "{col}". Each mask type can have a transfomer. Here's an example: https://github.com/apache/ranger/blob/72bccb3/agents-common/src/main/resources/service-defs/ranger-servicedef-hive.json#L374 Custom mask is one of the mask types. User can give customized transfomer in this type. 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 br Done. I edit these files in CLion. Can't find a place to customize this in its code style for Python... http://gerrit.cloudera.org:8080/#/c/14894/7/tests/authorization/test_ranger.py@796 PS7, Line 796: > nit: doesn't Done http://gerrit.cloudera.org:8080/#/c/14894/7/tests/authorization/test_ranger.py@832 PS7, Line 832: "CUSTOM", "concat({col}, 'ttt')") : policy_cnt += 1 > I would prefer to create a unique database for these to avoid the possibili good point! But we still need to use 'functional' as default db to test on resolving paths after masking. Will create these in unique_database -- 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: 9 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, 24 Dec 2019 05:16:10 +0000 Gerrit-HasComments: Yes
