Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/17199 )
Change subject: IMPALA-9661: Avoid introducing unused columns in table masking view ...................................................................... Patch Set 3: (7 comments) Thanks for working on this. Sending comments based on first pass. http://gerrit.cloudera.org:8080/#/c/17199/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/17199/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@491 PS3, Line 491: no user-visible effect Is the 'no user-visible effect' accurate though ? The plan in the query profile will show the effects of the table masking isn't it ? http://gerrit.cloudera.org:8080/#/c/17199/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@541 PS3, Line 541: reAnalyzeWithoutPrivChecks(stmtTableCache, authzCtx, origResultTypes, origColLabels); Shouldn't this only be called if reAnalyze = true ? I am also curious why this is being called again (line 506 has the previous invocation). http://gerrit.cloudera.org:8080/#/c/17199/3/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/17199/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@860 PS3, Line 860: column %s This says column but it seems the first parameter is resolvedTableRef's raw path ? http://gerrit.cloudera.org:8080/#/c/17199/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1326 PS3, Line 1326: names nit: this is registering a Column object rather than just the name. http://gerrit.cloudera.org:8080/#/c/17199/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1340 PS3, Line 1340: analyzer = analyzer.getParentAnalyzer(); Since getParentAnalyzer() can return null, we should break out of the loop if that happens. http://gerrit.cloudera.org:8080/#/c/17199/3/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java: http://gerrit.cloudera.org:8080/#/c/17199/3/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@171 PS3, Line 171: public boolean resolveTableMask(Analyzer analyzer) throws AnalysisException { Since this is not looking into the orderByElements_, I am wondering if it will miss the ORDER BY slot refs for table masking. http://gerrit.cloudera.org:8080/#/c/17199/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: http://gerrit.cloudera.org:8080/#/c/17199/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@268 PS3, Line 268: public boolean resolveTableMask(Analyzer analyzer) throws AnalysisException { Shouldn't this also consider the groupByClause_, havingClause_, aggregate and analyticInfo ? Are the columns referenced by those getting captured in some other manner ? -- To view, visit http://gerrit.cloudera.org:8080/17199 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib015a8ab528065907b27fbdceb8e2818deb814e1 Gerrit-Change-Number: 17199 Gerrit-PatchSet: 3 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Fri, 19 Mar 2021 06:31:52 +0000 Gerrit-HasComments: Yes
