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 34: (13 comments) http://gerrit.cloudera.org:8080/#/c/21142/27//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21142/27//COMMIT_MSG@9 PS27, Line 9: Adds "Select Columns", "Where Columns", "Join Columns", "Aggregate : Columns", and "OrderBy Columns" > Agreed. Done http://gerrit.cloudera.org:8080/#/c/21142/27//COMMIT_MSG@19 PS27, Line 19: : Testing was accomplished by adding/updat > Please mention Tests added/done for this patch. Done http://gerrit.cloudera.org:8080/#/c/21142/27/be/src/kudu/util/version_util.h File be/src/kudu/util/version_util.h: http://gerrit.cloudera.org:8080/#/c/21142/27/be/src/kudu/util/version_util.h@42 PS27, Line 42: // components transformed back into the string representation. The parser : // implementation has its quirks, so the canonical version string does not : // always match the raw input string. : std::string ToString() const; : : // The original version string. : std::string raw_version; : > All these changes can be done external to this file. Should be handled by https://gerrit.cloudera.org/c/21653 http://gerrit.cloudera.org:8080/#/c/21142/27/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21142/27/be/src/service/impala-server.cc@760 PS27, Line 760: in dumping " < > This was part of a rebase. Ack http://gerrit.cloudera.org:8080/#/c/21142/27/be/src/service/impala-server.cc@3235 PS27, Line 3235: LOG(INFO) << "Impala External Front > I agree. Deleted this line. Done http://gerrit.cloudera.org:8080/#/c/21142/27/be/src/service/internal-server.h File be/src/service/internal-server.h: http://gerrit.cloudera.org:8080/#/c/21142/27/be/src/service/internal-server.h@149 PS27, Line 149: > That line came from copying and pasting the comments from the ExecuteAndFet Done 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) { Does this need to propagate to ancestors_ Analyzers? http://gerrit.cloudera.org:8080/#/c/21142/34/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4573 PS34, Line 4573: dest.add(String.join(".", pathParts)); Please add TRACE log for any column addition. That will help debugging. http://gerrit.cloudera.org:8080/#/c/21142/34/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4589 PS34, Line 4589: List<SlotRef> slotRefs It seems like this List<SlotRef> can be transformed into Stream<Path> through map and filtering, and then feed into addColumnsTo(Set<String> dest, Stream<Path> paths). Have you try that? http://gerrit.cloudera.org:8080/#/c/21142/27/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/27/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@380 PS27, Line 380: : private void registerReferencedColumns() > Done Done 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: registerReferencedColumns(); Can we skip all column collections if WM is not enabled? http://gerrit.cloudera.org:8080/#/c/21142/34/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@389 PS34, Line 389: .filter(path -> path != null) nit: Can be .filter(Objects::nonNull) here and below? 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 { Is there any test against complex type column? It will be interesting too to have test query with at least two nested inline view. Probably using WITH clause. The point is to see how it looks if we have nested analyzer (see Analyzer.ancestors_). -- 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: 34 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 04:20:48 +0000 Gerrit-HasComments: Yes
