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 38: (8 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: /// Appends all relevant fields to a create or alter table sql statement. : /// Errors if no columns were added to the sql stream. : static void _appendCols(StringStreamPop& sql, > Done Done 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 the future we want to do other kind of ALTER TABLE (see "Schema evolution of Iceberg tables" in https://impala.apache.org/docs/build/html/topics/impala_iceberg.html)? Or upgrading the iceberg table version? ALTER TABLE ice_v1_to_v2 SET TBLPROPERTIES('format-version'='2'); What if we organize each version changes into their own function? Let say, call this function _upgradeTable_VERSION_1_1_0(). 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? 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: > Done Done 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? Can you create a static boolean method to wrap this under Exprs.java and document them? 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. 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? 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 { > I'm following you now. I did a test, and the column sub items (such as db. Done -- 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 <[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: Fri, 23 Aug 2024 22:40:48 +0000 Gerrit-HasComments: Yes
