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 38: (5 comments) http://gerrit.cloudera.org:8080/#/c/21142/38/be/src/service/workload-management-init.cc File be/src/service/workload-management-init.cc: http://gerrit.cloudera.org:8080/#/c/21142/38/be/src/service/workload-management-init.cc@166 PS38, Line 166: cols_to_add << "ALTER TABLE " << table_name << " ADD IF NOT EXISTS COLUMNS("; : _appendCols(cols_to_add, [current_version, target_version](const FieldDefinition& f){ : return f.schema_version > current_version && f.schema_version <= target_version; }); : cols_to_add << ")"; > This seems OK if version upgrade is only adding new columns. But what if in My original implementation had the upgrade broken out by version. I moved away from that implementation because I prefer this more generic approach that will work for any version upgrade provided each upgrade is only adding columns. I want to keep this code generic for now but, in the future, will break out the upgrades into their own function per version should that be needed. http://gerrit.cloudera.org:8080/#/c/21142/38/be/src/statestore/statestore.h File be/src/statestore/statestore.h: http://gerrit.cloudera.org:8080/#/c/21142/38/be/src/statestore/statestore.h@242 PS38, Line 242: /// Topic coordinating the initialization of workload management on all coordinators. : static const char* IMPALA_WORKLOAD_MANAGEMENT_TOPIC; > This is unused now? Good catch. Not sure how this snuck back in, most likely due to a conflict when rebasing onto https://gerrit.cloudera.org/#/c/21653/ Removed here and in statestore.cc http://gerrit.cloudera.org:8080/#/c/21142/38/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/38/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4662 PS38, Line 4662: sourceExpr instanceof FunctionCallExpr : || sourceExpr instanceof CaseExpr : || sourceExpr instanceof ArithmeticExpr : || sourceExpr instanceof CastExpr > Why this check is limited to these four Expr subclass only? I tested this by adding an else clause and running the entire tpcds suite of queries through. These four were the only Expr subclasses that warranted special handling. The reason these four are called out is they have child slot refs in the form of arguments that need to be included. For example, give the query 'select min(mycol)', min is an instance of the FunctionCallExpr class with child slot refs of 'mycol' which we need to record as being used in the select columns. I like the idea of adding a method that can be called instead of having an arbitrary list here. I have added such a method. http://gerrit.cloudera.org:8080/#/c/21142/38/fe/src/main/java/org/apache/impala/analysis/Path.java File fe/src/main/java/org/apache/impala/analysis/Path.java: http://gerrit.cloudera.org:8080/#/c/21142/38/fe/src/main/java/org/apache/impala/analysis/Path.java@442 PS38, Line 442: public List<String> getFullyQualifiedRawPath(boolean preferAlias) > Please add documentation and describe what preferAlias means. Done http://gerrit.cloudera.org:8080/#/c/21142/38/fe/src/main/java/org/apache/impala/analysis/Path.java@450 PS38, Line 450: rootTable_ instanceof IcebergMetadataTable > Why is this changed from VirtualTable? The ColumnsTest junits revealed that Iceberg delete tables caused an error here because the IcebergDeleteTable class is an instanceof VirtualTable but not IcebergMetadataTable, thus the cast threw an exception. -- 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: 38 Gerrit-Owner: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Wed, 28 Aug 2024 17:49:23 +0000 Gerrit-HasComments: Yes
