Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/16976 )
Change subject: IMPALA-9234: Support Ranger row filtering policies ...................................................................... Patch Set 9: (4 comments) A few comments below. Overall this looks good. http://gerrit.cloudera.org:8080/#/c/16976/9/be/src/util/backend-gflag-util.cc File be/src/util/backend-gflag-util.cc: http://gerrit.cloudera.org:8080/#/c/16976/9/be/src/util/backend-gflag-util.cc@158 PS9, Line 158: DEFINE_bool(enable_row_filtering, true, Since enabling this flag depends on enabling column masking, it would be useful to show something like 'enabling this flag requires enable_column_masking to be true'. http://gerrit.cloudera.org:8080/#/c/16976/9/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/9/fe/src/main/java/org/apache/impala/authorization/TableMask.java@109 PS9, Line 109: String stmtSql = String.format("SELECT * FROM %s.%s WHERE %s", My understanding is the SELECT * will need to be changed to only project columns referenced in the query. If that's true and there's an existing JIRA for it, you can reference it here. http://gerrit.cloudera.org:8080/#/c/16976/9/fe/src/main/java/org/apache/impala/authorization/TableMask.java@111 PS9, Line 111: SelectStmt selectStmt = (SelectStmt) Parser.parse(stmtSql); Since this will throw an AnalysisException (or ParseException) for a malformed row filter, would that accidentally show the column name ? http://gerrit.cloudera.org:8080/#/c/16976/9/testdata/workloads/functional-query/queries/QueryTest/ranger_row_filtering.test File testdata/workloads/functional-query/queries/QueryTest/ranger_row_filtering.test: http://gerrit.cloudera.org:8080/#/c/16976/9/testdata/workloads/functional-query/queries/QueryTest/ranger_row_filtering.test@1 PS9, Line 1: ==== Thanks for adding these tests. I had a couple of suggestions to the test coverage and a question about EXPLAIN: 1. A test for AnalyticException/ParseException for a malformed row filter. What is actually shown in such a error message and do we need to hide anything in the message. 2. Since WITH clause and VIEWs are expected to also be enforcing the row filter, could you add one test for each ? 3. (this is more of a question) What about EXPLAIN ? Will an EXPLAIN for a query that has table masking applied (either row/column masking or both) show the rewritten query plan ? What about the runtime query profile ? Depending on the expected behavior, we should probably add a sanity test. -- 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: 9 Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.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, 17 Mar 2021 01:49:28 +0000 Gerrit-HasComments: Yes
