Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17530 )
Change subject: IMPALA-5569: Add statement ALTER TABLE UNSET TBLPROPERTIES/SERDEPROPERTIES ...................................................................... Patch Set 2: (15 comments) The change looks great, I've mostly found nits. http://gerrit.cloudera.org:8080/#/c/17530/2/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/17530/2/common/thrift/JniCatalog.thrift@391 PS2, Line 391: ('k1', 'k2'...) nit: ('k1'='a', 'k2'='b'...) http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/main/java/org/apache/impala/analysis/AlterTableUnSetTblProperties.java File fe/src/main/java/org/apache/impala/analysis/AlterTableUnSetTblProperties.java: http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/main/java/org/apache/impala/analysis/AlterTableUnSetTblProperties.java@63 PS2, Line 63: nit: +1 indent http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/main/java/org/apache/impala/analysis/AlterTableUnSetTblProperties.java@116 PS2, Line 116: if (KuduTable.isSynchronizedTable(table_.getMetaStoreTable())) { : AnalysisUtils.throwIfTrue(tblPropertyKeys_.contains(KuduTable.KEY_TABLE_NAME), : String.format("Not allowed to unset '%s' for synchronized Kudu tables .", : KuduTable.KEY_TABLE_NAME)); nit: Can we use the following instead? propertyCheck(KuduTable.KEY_TABLE_NAME, "synchronized Kudu"); http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/main/java/org/apache/impala/analysis/AlterTableUnSetTblProperties.java@121 PS2, Line 121: List<String> unsupportedKuduProperties = Lists.newArrayList(KuduTable.KEY_TABLE_ID, : KuduTable.KEY_MASTER_HOSTS); : for (String unsupportedProperty: unsupportedKuduProperties) { : // Throw error if unsupportedProperty is provided. : AnalysisUtils.throwIfTrue(tblPropertyKeys_.contains(unsupportedProperty), : String.format("Property '%s' cannot be unset for Kudu tables", : unsupportedProperty)); : } nit: can we use the following instead? propertyCheck(KuduTable.KEY_TABLE_ID, "Kudu"); propertyCheck(KuduTable.KEY_MASTER_HOSTS, "Kudu"); http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@873 PS2, Line 873: nit: too much indentation http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@877 PS2, Line 877: nit: too much indent http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@871 PS2, Line 871: case UNSET_TBL_PROPERTIES: : alterTableUnSetTblProperties(tbl, params.getUnset_tbl_properties_params(), : numUpdatedPartitions); : reloadTableSchema = true; : if (params.getUnset_tbl_properties_params().isSetPartition_set()) { : addSummary(response, : "Updated " + numUpdatedPartitions.getRef() + " partition(s)."); : } else { : addSummary(response, "Updated table."); : } nit: I think this could be moved to just after SET_TBL_PROPERTIES http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3790 PS2, Line 3790: nit: too much indent http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3801 PS2, Line 3801: " nit: too much indent http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3818 PS2, Line 3818: removeProperties.removeAll(keys); This modifies 'TAlterTableUnSetTblPropertiesParams params' which might cause strange behavior later. Since we are on an error path here, I think we could just have a copy of removeProperties and modify that. http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1162 PS2, Line 1162: nit: too much indent http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1166 PS2, Line 1166: nit: too much indent http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java: http://gerrit.cloudera.org:8080/#/c/17530/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@687 PS2, Line 687: // Unsetting kudu.table_id is not allowed for Kudu tables. : AnalysisError("ALTER TABLE functional_kudu.testtbl UNSET " + : "TBLPROPERTIES ('kudu.table_id')", : "Property 'kudu.table_id' cannot be unset for Kudu tables"); : : // Unsetting kudu.master_addresses is not allowed for Kudu tables. : AnalysisError("ALTER TABLE functional_kudu.testtbl UNSET " + : "TBLPROPERTIES ('kudu.master_addresses')", : "Property 'kudu.master_addresses' cannot be unset for Kudu tables"); Indentation is a bit messy http://gerrit.cloudera.org:8080/#/c/17530/2/testdata/workloads/functional-query/queries/QueryTest/alter-table.test File testdata/workloads/functional-query/queries/QueryTest/alter-table.test: http://gerrit.cloudera.org:8080/#/c/17530/2/testdata/workloads/functional-query/queries/QueryTest/alter-table.test@1824 PS2, Line 1824: functional_avro.alltypes Tables in the functional dataset are used by other tests as well, maybe even in parallel. So it is better if we not modify them, only their copy, or create an external table that points to them and change the table property of the external table. http://gerrit.cloudera.org:8080/#/c/17530/2/testdata/workloads/functional-query/queries/QueryTest/alter-table.test@1845 PS2, Line 1845: ==== Could you please add a few tests where you are unsetting multiple properties? E.g.: * all props exist * some exist, some not * with and without 'if exists' -- To view, visit http://gerrit.cloudera.org:8080/17530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ife4f6561dcdcd20c76eb299c6661c778e342509d Gerrit-Change-Number: 17530 Gerrit-PatchSet: 2 Gerrit-Owner: Amogh Margoor <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 03 Jun 2021 16:58:23 +0000 Gerrit-HasComments: Yes
