Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21142 )
Change subject: IMPALA-12737: List columns in profile and query history ...................................................................... Patch Set 26: (5 comments) http://gerrit.cloudera.org:8080/#/c/21142/26//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21142/26//COMMIT_MSG@9 PS26, Line 9: "Select Columns", "Where Columns", "Join Columns", "Aggregate : Columns", and "OrderBy Columns" Should this add "Having Columns" as well? http://gerrit.cloudera.org:8080/#/c/21142/26/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/21142/26/common/thrift/Frontend.thrift@705 PS26, Line 705: // Columns referenced in a select list. : 21: optional list<string> select_columns : : // Columns referenced in a where clause. : 22: optional list<string> where_columns : : // Columns referenced in a join clause. : 23: optional list<string> join_columns : : // Columns referenced in an aggregation. : 24: optional list<string> aggregate_columns : : // Columns referenced in an order by clause. : 25: optional list<string> orderby_columns I think there should be limit on maximum column names to log, or total length of the strings. If selecting hundreds of columns, it is probably not useful to log everything. http://gerrit.cloudera.org:8080/#/c/21142/26/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/21142/26/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4488 PS26, Line 4488: Stream<Path> Is there benefit of passing Stream<Path> argument instead of Set<String>? To me, Set<String> is final, and it is caller responsibility to do all necessary transformation and filtering. http://gerrit.cloudera.org:8080/#/c/21142/26/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/21142/26/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@383 PS26, Line 383: Stream<SelectListItem> nonStarItems = : selectList_.getItems().stream().filter(elem -> !elem.isStar()); : nonStarItems.forEach(item -> item.getExpr().collect(SlotRef.class, slotRefs)); Can be combined into one? http://gerrit.cloudera.org:8080/#/c/21142/26/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@387 PS26, Line 387: .filter(path -> path != null) Why is the filtering applied here and not in the concatenated Stream? -- To view, visit http://gerrit.cloudera.org:8080/21142 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I78f3670b067c0c192ee8a212fba95466fbcb51d7 Gerrit-Change-Number: 21142 Gerrit-PatchSet: 26 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Mon, 15 Apr 2024 19:32:40 +0000 Gerrit-HasComments: Yes
