Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16976 )
Change subject: [WIP] IMPALA-9234: Support Ranger row filtering policies ...................................................................... Patch Set 1: (4 comments) 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: place I think we need to add some more tests for different row filter types, e.g. more complex expressions, invalid expressions, etc. Are there any restrictions on the types of expressions that can be used in row filters? E.g. are UDFs allowed? 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: String stmtSql = String.format("SELECT * FROM %s.%s WHERE %s", We might need to be a bit careful with testing invalid row filters that don't parse, etc. It seems like this approach should work, but just trying to think about the risks. 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: List<AuthzAuditEvent> auditEvents = auditHandler.getAuthzEvents(); Can we factor out this code and share it with the similar logic above? This code seems subtle and it would be good to only have one implementation. 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: * We stash the List of AuthzAuditEvent's in a Map after the analysis of the query and This comment probably needs an update. I find this method a bit confusing overall. Maybe it would be clearer if the comment described what it means to call this method? Or when it would be called? The comment seems to mainly discuss the implementation. -- 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: 1 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 27 Jan 2021 20:42:27 +0000 Gerrit-HasComments: Yes
