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 52: (11 comments) I have not review this change thoroughly since ps40, but I want to leave my initial review. http://gerrit.cloudera.org:8080/#/c/21142/52/be/src/catalog/catalogd-main.cc File be/src/catalog/catalogd-main.cc: http://gerrit.cloudera.org:8080/#/c/21142/52/be/src/catalog/catalogd-main.cc@105 PS52, Line 105: ABORT_IF_ERROR(catalog_server.InitWorkloadManagement()); : Maybe should check catalog_server.IsActive() in case CatalogD HA is enabled. http://gerrit.cloudera.org:8080/#/c/21142/52/be/src/catalog/workload-management-init.cc File be/src/catalog/workload-management-init.cc: http://gerrit.cloudera.org:8080/#/c/21142/52/be/src/catalog/workload-management-init.cc@188 PS52, Line 188: CREATE [EXTERNAL] TABLE IF NOT EXISTS {{table_name}} ... Expand this fully to also include "STORED AS ICEBERG", etc. http://gerrit.cloudera.org:8080/#/c/21142/52/be/src/catalog/workload-management-init.cc@241 PS52, Line 241: table_props.insert(make_pair("engine.hive.enabled", "true")); : table_props.insert(make_pair("write.delete.mode", "merge-on-read")); : table_props.insert(make_pair("write.format.default", "parquet")); : table_props.insert(make_pair("write.merge.mode", "merge-on-read")); : table_props.insert(make_pair("write.parquet.compression-codec", "snappy")); : table_props.insert(make_pair("write.update.mode", "merge-on-read")); Why is this not a default value for FLAGS_query_log_table_props? What if user want to override some of the optional one, like snappy to something else? http://gerrit.cloudera.org:8080/#/c/21142/52/be/src/catalog/workload-management-init.cc@248 PS52, Line 248: Table partioning nit: Iceberg table partitioning http://gerrit.cloudera.org:8080/#/c/21142/52/be/src/catalog/workload-management-init.cc@297 PS52, Line 297: /// Equivalent to: ALTER TABLE {{table_name}} ADD IF NOT EXISTS COLUMNS (...). Also document the second ALTER to change schema version property. http://gerrit.cloudera.org:8080/#/c/21142/52/be/src/workload_mgmt/workload-management.h File be/src/workload_mgmt/workload-management.h: http://gerrit.cloudera.org:8080/#/c/21142/52/be/src/workload_mgmt/workload-management.h@196 PS52, Line 196: {TQueryTableColumn::SELECT_COLUMNS, FieldDefinition(TPrimitiveType::STRING, : VERSION_1_1_0)}, Add blank space or comment to separate the next columns from VERSION_1_1_0 columns. http://gerrit.cloudera.org:8080/#/c/21142/52/common/thrift/SystemTables.thrift File common/thrift/SystemTables.thrift: http://gerrit.cloudera.org:8080/#/c/21142/52/common/thrift/SystemTables.thrift@21 PS52, Line 21: # Must be kept in-sync with workload-management-fields.cc : # Used as column names, so do not change existing enums. : # When adding new columns, review the default for query_log_max_queued to maintain : # query_log_max_queued * len(TQueryTableColumn) < statement_expression_limit(250k) nit: Would be great to have CHANGELOG documented here for every schema update. http://gerrit.cloudera.org:8080/#/c/21142/52/common/thrift/SystemTables.thrift@75 PS52, Line 75: SELECT_COLUMNS Please add comment just before L75 to say that these are new columns introduced by VERSION_1_1_0. http://gerrit.cloudera.org:8080/#/c/21142/40/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/21142/40/fe/src/main/java/org/apache/impala/analysis/Expr.java@2041 PS40, Line 2041: recordChildrenInWork > Agreed this function name is not clear enough. Renamed. Done http://gerrit.cloudera.org:8080/#/c/21142/52/testdata/workload_mgmt/create_impala_query_live_table.sql File testdata/workload_mgmt/create_impala_query_live_table.sql: http://gerrit.cloudera.org:8080/#/c/21142/52/testdata/workload_mgmt/create_impala_query_live_table.sql@56 PS52, Line 56: nit: trailing whitespace. http://gerrit.cloudera.org:8080/#/c/21142/52/tests/custom_cluster/test_workload_mgmt_init.py File tests/custom_cluster/test_workload_mgmt_init.py: http://gerrit.cloudera.org:8080/#/c/21142/52/tests/custom_cluster/test_workload_mgmt_init.py@64 PS52, Line 64: catalog_opts = "--catalogd_args=--enable_workload_mgmt " Can you test --enable_catalogd_ha as well? See https://gerrit.cloudera.org/c/21615/ for documentation. -- 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: 52 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: Mon, 14 Oct 2024 23:13:25 +0000 Gerrit-HasComments: Yes
