Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18953 )
Change subject: IMPALA-11565: Support IF NOT EXISTS in alter table add columns for kudu/iceberg table ...................................................................... Patch Set 15: (9 comments) http://gerrit.cloudera.org:8080/#/c/18953/15//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18953/15//COMMIT_MSG@10 PS15, Line 10: patch nit: line too long http://gerrit.cloudera.org:8080/#/c/18953/15/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/18953/15/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1040 PS15, Line 1040: may cause nit: that may cause http://gerrit.cloudera.org:8080/#/c/18953/15/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1281 PS15, Line 1281: addSummary(response, "Column(s) have been added."); Should we return response with "No new column(s) have been added to the table" if there is no column to be added? It's better to keep consistent in the behavior as line #1048 - 1052. http://gerrit.cloudera.org:8080/#/c/18953/15/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1351 PS15, Line 1351: addSummary(response, "Column(s) have been added."); Should we return response with "No new column(s) have been added to the table." if there is no column to be added? http://gerrit.cloudera.org:8080/#/c/18953/15/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/18953/15/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@736 PS15, Line 736: a string null similar to line #734. It's better to add two or three columns here. http://gerrit.cloudera.org:8080/#/c/18953/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-alter.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-alter.test: http://gerrit.cloudera.org:8080/#/c/18953/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-alter.test@460 PS15, Line 460: 'Column(s) have been added.' This message is misleading http://gerrit.cloudera.org:8080/#/c/18953/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-alter.test@465 PS15, Line 465: Add a column that already exists Only one column 'b' is added in line 467 http://gerrit.cloudera.org:8080/#/c/18953/15/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test File testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test: http://gerrit.cloudera.org:8080/#/c/18953/15/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test@395 PS15, Line 395: Recover status before add 'new_col5' column 'new_col6' was added after 'new_col5' was added. This does not restore to the status before adding 'new_col5' column. You should drop 'new_col6' first, or update the comment. http://gerrit.cloudera.org:8080/#/c/18953/15/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test@420 PS15, Line 420: ---- QUERY add more test cases, like alter table tbl_to_alter add if not exists columns (new_col3 string, new_col4 int); // all new columns are existing columns. alter table tbl_to_alter add if not exists columns (new_col7 string, new_col7 int); // duplicated columns, only the first one is added -- To view, visit http://gerrit.cloudera.org:8080/18953 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82590e5372e881f2e81d4ed3dd0d32a2d3ddb517 Gerrit-Change-Number: 18953 Gerrit-PatchSet: 15 Gerrit-Owner: Baike Xia <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Baike Xia <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jian Zhang <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Wed, 01 Mar 2023 01:01:04 +0000 Gerrit-HasComments: Yes
