Jason Fehr 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: (7 comments) 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; : > Should be handled by https://gerrit.cloudera.org/c/21653 Yes, that patch handles adding the necessary operators via a new file not under the kudu directory tree. The latest revision of this patch does not have any changes to any version util files. 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? I don't believe so because callers provide a variable from the globalState, and the workload management data is build from that globalState. 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. Done 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> throu Yes it can. I first re-wrote the code to run through the slotRefs list once with the filter operation either returning true or returning false and calling the resolveActualPath function. I modified that because I did not like having logic dependent on a side-effect of the filter operation. 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? Done 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? 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 { > Is there any test against complex type column? The test_workload_mgmt_sql_details.py runs a few of the more complicated TPCDS queries. None of them test complex types though. I don't see complex types causing issues because workload management only cares about the column name, not its type. I could be wrong though if complex type columns have different objects (outside of the SlotRef class hierarchy) representing them in the planner. -- 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 22:48:03 +0000 Gerrit-HasComments: Yes
