Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21142 )
Change subject: IMPALA-12737: Query columns in workload management tables. ...................................................................... Patch Set 37: (7 comments) http://gerrit.cloudera.org:8080/#/c/21142/37/be/src/service/workload-management-init.cc File be/src/service/workload-management-init.cc: http://gerrit.cloudera.org:8080/#/c/21142/37/be/src/service/workload-management-init.cc@85 PS37, Line 85: /// Returns true if at least one column was added to the sql stream, false otherwise. : static void _appendCols(StringStreamPop& sql, : std::function<bool(const FieldDefinition& item)> shouldIncludeCol) { Says "Return true if..." but method is static void. http://gerrit.cloudera.org:8080/#/c/21142/34/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/34/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4565 PS34, Line 4565: private void addColumnsTo(Set<String> dest, Stream<Path> paths) { > I don't believe so because callers provide a variable from the globalState, Ack http://gerrit.cloudera.org:8080/#/c/21142/34/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4573 PS34, Line 4573: String fullPath = String.join(".", pat > Done Done http://gerrit.cloudera.org:8080/#/c/21142/34/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4589 PS34, Line 4589: } will be added to thi > Yes it can. I first re-wrote the code to run through the slotRefs list once Ack http://gerrit.cloudera.org:8080/#/c/21142/37/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/37/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4576 PS37, Line 4576: LOG.trace("Adding column {}", fullPath) Please mention which set this column is added to (select, where, join, etc). http://gerrit.cloudera.org:8080/#/c/21142/34/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/34/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@378 PS34, Line 378: analyzer_.setSimpleLimitStat > Done Done http://gerrit.cloudera.org:8080/#/c/21142/34/fe/src/test/java/org/apache/impala/planner/ColumnsTest.java File fe/src/test/java/org/apache/impala/planner/ColumnsTest.java: http://gerrit.cloudera.org:8080/#/c/21142/34/fe/src/test/java/org/apache/impala/planner/ColumnsTest.java@38 PS34, Line 38: public class ColumnsTest extends FrontendTestBase { > The test_workload_mgmt_sql_details.py runs a few of the more complicated TP I'm asking because it looks like Path of complex type can ends with '.item' or '.pos', like documented here: https://github.com/apache/impala/blob/76847fb03d9cc92530f97517a4481993392f331a/fe/src/main/java/org/apache/impala/analysis/Path.java#L76-L83 So for the purpose of this patch, d.t.c path is OK, but d.t.c.item or d.t.c.pos is not. -- 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: 37 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: Thu, 22 Aug 2024 23:56:13 +0000 Gerrit-HasComments: Yes
