Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18514 )
Change subject: IMPALA-801, IMPALA-8011: Add INPUT__FILE__NAME virtual column for file name ...................................................................... Patch Set 3: (4 comments) The patch looks pretty good to me! I'm going to do another round of more detailed review. Left some comments first. http://gerrit.cloudera.org:8080/#/c/18514/3/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/18514/3/be/src/exec/hdfs-scan-node-base.cc@432 PS3, Line 432: nit: no indention? http://gerrit.cloudera.org:8080/#/c/18514/3/fe/src/main/java/org/apache/impala/analysis/Path.java File fe/src/main/java/org/apache/impala/analysis/Path.java: http://gerrit.cloudera.org:8080/#/c/18514/3/fe/src/main/java/org/apache/impala/analysis/Path.java@279 PS3, Line 279: if (rootTable_ == null) return false; : if (rootDesc_ != null) { : if (rootDesc_.getType() != rootTable_.getType().getItemType()) { : // 'rootDesc_' describes a collection tuple. Currently we only allow virtual : // columns at the table-level. : return false; : } : } : if (rawPath_.size() != 1) return false; Can we add some test coverage in AnalyzerTest for these? http://gerrit.cloudera.org:8080/#/c/18514/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/18514/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@633 PS3, Line 633: getVirtualColumns().clear(); Can we do this in Table#clearColumns()? The function name doesn't indicate the clear. http://gerrit.cloudera.org:8080/#/c/18514/3/testdata/workloads/functional-query/queries/QueryTest/virtual-column-input-file-name.test File testdata/workloads/functional-query/queries/QueryTest/virtual-column-input-file-name.test: http://gerrit.cloudera.org:8080/#/c/18514/3/testdata/workloads/functional-query/queries/QueryTest/virtual-column-input-file-name.test@4 PS3, Line 4: select input__file__name, * from alltypestiny; Can we copy some tests here to testdata/workloads/functional-query/queries/QueryTest/ranger_column_masking.test? I saw a error like this: I0512 17:16:11.198302 24544 jni-util.cc:286] ee40ba3ebe04e6c1:e912b58200000000] java.lang.IllegalStateException at com.google.common.base.Preconditions.checkState(Preconditions.java:492) at org.apache.impala.analysis.StatementBase.castResultExprs(StatementBase.java:114) at org.apache.impala.analysis.AnalysisContext.reAnalyze(AnalysisContext.java:620) at org.apache.impala.analysis.AnalysisContext.analyze(AnalysisContext.java:542) at org.apache.impala.analysis.AnalysisContext.analyzeAndAuthorize(AnalysisContext.java:468) at org.apache.impala.service.Frontend.doCreateExecRequest(Frontend.java:2018) at org.apache.impala.service.Frontend.getTExecRequest(Frontend.java:1926) at org.apache.impala.service.Frontend.createExecRequest(Frontend.java:1750) at org.apache.impala.service.JniFrontend.createExecRequest(JniFrontend.java:164) I feel like we don't need to support virtual columns when the table has Ranger column-masking/row-filtering policies on the current user. Because the user is not a privileged user so should not be aware of the internal details (e.g. file names) of this table. So we just need to fail the query elegantly. -- To view, visit http://gerrit.cloudera.org:8080/18514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I498591f1db08a91a5c846df59086d2291df4ff61 Gerrit-Change-Number: 18514 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Gergely Fürnstáhl <gfurnst...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 12 May 2022 11:44:22 +0000 Gerrit-HasComments: Yes
