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 2: (4 comments) Thanks Tim's comments! This patch is ready for review now. Also added Csaba and Fang-Yu as reviewers since you are involved in the work of column masking. Hope you can take a look when you have time. Thanks! http://gerrit.cloudera.org:8080/#/c/16976/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16976/1//COMMIT_MSG@33 PS1, Line 33: Tests: > I think we need to add some more tests for different row filter types, e.g. Yes, UDFs are allowed. Ideally, we should support all kinds of expressions. However, after adding more tests, I found two hard cases and would support them in follow-up patches: 1) Expressions that contains subqueries on other tables. Those tables can't be resolved correctly since table metadata loading is done before analyzing. E.g. for table "functional.alltypesagg" we can create a row-filter policy with expr: "id in (select id from functional.alltypestiny)". It's expected to transform query "select * from functional.alltypesagg" to "select * from functional.alltypesagg where id in (select id from functional.alltypestiny)". The query fails since the analyzer can't resolve "functional.alltypestiny" which is not in the StmtTableCache. Filed IMPALA-10483. 2) Expressions tables containing nested types and the query uses unrelated collection column directly. We need to rewrite the query to apply the table masking view on the base table. Filed IMPALA-10484. http://gerrit.cloudera.org:8080/#/c/16976/1/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/16976/1/fe/src/main/java/org/apache/impala/authorization/TableMask.java@102 PS1, Line 102: /** > We might need to be a bit careful with testing invalid row filters that don Yeah, but we can only detect syntax errors here. For semantic errors like return type is not BOOLEAN, we need to detect it later. I'll add some tests later. http://gerrit.cloudera.org:8080/#/c/16976/1/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/1/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@397 PS1, Line 397: maskedColumn = maskedValue.replace("{col}", columnName); > Can we factor out this code and share it with the similar logic above? This Done. We still need to revisit whether row filtering policies will produce stale events (like column masking policies). http://gerrit.cloudera.org:8080/#/c/16976/1/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationContext.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationContext.java: http://gerrit.cloudera.org:8080/#/c/16976/1/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationContext.java@56 PS1, Line 56: * Stash and deduplicate the audit events produced by table masking (Column-masking / > This comment probably needs an update. I find this method a bit confusing o Done. Refactor these to be more general. @Fang-Yu Please help to reivew them since you are the author :) -- 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: 2 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: Mon, 08 Feb 2021 12:44:42 +0000 Gerrit-HasComments: Yes
