You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Baike Xia (Code Review)" <ge...@cloudera.org> on 2022/09/08 16:55:00 UTC

[Impala-ASF-CR] IMPALA-11565: For KUDU tables, alter table add column operation is optimized

Baike Xia has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18953


Change subject: IMPALA-11565: For KUDU tables, alter table add column operation is optimized
......................................................................

IMPALA-11565: For KUDU tables, alter table add column operation is optimized

For kudu table, add/replace if not exists column operation,
if the columns already exist and are of the same type,
no operation is performed;
If the type is different, an error is reported.

Change-Id: I82590e5372e881f2e81d4ed3dd0d32a2d3ddb517
---
M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
2 files changed, 29 insertions(+), 3 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/18953/2
-- 
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: newchange
Gerrit-Change-Id: I82590e5372e881f2e81d4ed3dd0d32a2d3ddb517
Gerrit-Change-Number: 18953
Gerrit-PatchSet: 2
Gerrit-Owner: Baike Xia <xi...@163.com>

[Impala-ASF-CR] IMPALA-11565: For alter table, add column operation is optimized

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18953 )

Change subject: IMPALA-11565: For alter table, add column operation is optimized
......................................................................


Patch Set 4:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/11432/ : Initial code review checks failed. See linked job for details on the failure.


-- 
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: 4
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Comment-Date: Mon, 26 Sep 2022 07:58:03 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11565: For alter table, add column operation is optimized

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18953 )

Change subject: IMPALA-11565: For alter table, add column operation is optimized
......................................................................


Patch Set 6:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/11434/ : Initial code review checks failed. See linked job for details on the failure.


-- 
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: 6
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Comment-Date: Mon, 26 Sep 2022 08:41:13 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11565: Support IF NOT EXISTS in alter table add columns for kudu/iceberg table

Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.

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 16: Code-Review+2


-- 
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: 16
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Mar 2023 16:51:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11565: Support IF NOT EXISTS in alter table add columns for kudu/iceberg table

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins 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 13:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12073/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 13
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 30 Dec 2022 04:29:50 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11565: Support IF NOT EXISTS in alter table add columns for kudu/iceberg table

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins 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 16:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9085/ DRY_RUN=true


-- 
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: 16
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Mar 2023 09:33:07 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11565: Support IF NOT EXISTS in alter table add columns for kudu/iceberg table

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.

lipenglin@apache.org 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 14: Code-Review+1


-- 
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: 14
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Mon, 30 Jan 2023 09:50:27 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11565: For alter table, add column operation is optimized

Posted by "Baike Xia (Code Review)" <ge...@cloudera.org>.

Baike Xia has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/18953 )

Change subject: IMPALA-11565: For alter table, add column operation is optimized
......................................................................

IMPALA-11565: For alter table, add column operation is optimized

For alter table, add if not exists column operation,
if the columns already exist and are of the same type,
no operation is performed;
If the type is different, an error is reported.

Change-Id: I82590e5372e881f2e81d4ed3dd0d32a2d3ddb517
---
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
4 files changed, 40 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/18953/7
-- 
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: newpatchset
Gerrit-Change-Id: I82590e5372e881f2e81d4ed3dd0d32a2d3ddb517
Gerrit-Change-Number: 18953
Gerrit-PatchSet: 7
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>

[Impala-ASF-CR] IMPALA-11565: Support IF NOT EXISTS in alter table add columns for kudu table

Posted by "Baike Xia (Code Review)" <ge...@cloudera.org>.

Baike Xia 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 table
......................................................................


Patch Set 8:

(1 comment)

> Patch Set 8: Verified+1

http://gerrit.cloudera.org:8080/#/c/18953/8/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java:

http://gerrit.cloudera.org:8080/#/c/18953/8/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java@87
PS8, Line 87:         throw new AnalysisException("Duplicate column name: " + colName);
> should we also ignore this error when `if not exists` is presented?
It's already there, and It works for this situation 'alter table tbl add if not exists columns (b int, b int);', but this might be debatable if the field types were different. e.g.: alter table tbl add if not exists columns (b int, b string);



-- 
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: 8
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Sun, 09 Oct 2022 02:27:08 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11565: For alter table, add column operation is optimized

Posted by "Jian Zhang (Code Review)" <ge...@cloudera.org>.

Jian Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18953 )

Change subject: IMPALA-11565: For alter table, add column operation is optimized
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/18953/3/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java:

http://gerrit.cloudera.org:8080/#/c/18953/3/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java@87
PS3, Line 87:       if (ifNotExists_) {
            :         if (col != null) {
            :           if (!col.getType().equals(c.getType())) {
            :             throw new AnalysisException(String.format("Error adding column %s " +
            :                 "from table %s: type not match", colName, t.getName()));
            :           } else {
            :             iterator.remove();
            :           }
            :         }
            :       } else if (col != null) {
            :         throw new AnalysisException("Column already exists: " + colName);
            :       }
how about simplifying the precondition check to the following to improve the code readability:

```cpp
if (col != nil) {
  if (!ifNotExists) {
    throw new AnalysisException("Column already exists: " + colName);
  }
  
  // handle the case that ifNotExists is true
  if (!col.getType().equals(c.getType())) {
    throw new AnalysisException(String.format("Error adding column %s " +
        "from table %s: type not match", colName, t.getName()));
  }
  
  // remove the column to be added which is existed.
  iterator.remove();
}
```


http://gerrit.cloudera.org:8080/#/c/18953/3/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/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1029
PS3, Line 1029:           if (params.getAdd_cols_params() != null
              :               && params.getAdd_cols_params().getColumnsSize() != 0) {
could you add a coment about the reason for adding this validation, is it a bug-fix?



-- 
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: 3
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Comment-Date: Tue, 13 Sep 2022 11:28:58 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11565: For alter table, add column operation is optimized

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.

Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18953 )

Change subject: IMPALA-11565: For alter table, add column operation is optimized
......................................................................


Patch Set 7:

(9 comments)

I left some comments before I realized this is actually a behavior change. Currently the DDL won't fail if IF-NOT-EXISTS is specified. After this patch, it could fail if the column exists with a different type. The current behavior makes more sense to me, because IF-NOT-EXISTS is usually used to assure the success of the statement. I think we need a broader discussion for whether we need this patch.

Looking into other systems, Hive and MySQL don't support IF-NOT-EXISTS in AlterTableAddColumns. PostgreSQL supports such syntax but it won't check the type of the existing column.

https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL#LanguageManualDDL-Add/ReplaceColumns
https://dev.mysql.com/doc/refman/5.7/en/alter-table.html
https://www.postgresql.org/docs/current/sql-altertable.html

http://gerrit.cloudera.org:8080/#/c/18953/7//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/18953/7//COMMIT_MSG@7
PS7, Line 7: For alter table, add column operation is optimized
Let's make the title more specific, e.g. "Check column types for ALTER TABLE ADD IF NOT EXISTS COLUMN operation"


http://gerrit.cloudera.org:8080/#/c/18953/7//COMMIT_MSG@9
PS7, Line 9: alter table, add if not exists column
nit: reword to "ALTER TABLE ADD IF NOT EXISTS COLUMN"


http://gerrit.cloudera.org:8080/#/c/18953/7//COMMIT_MSG@10
PS7, Line 10: and are of
nit: "with"


http://gerrit.cloudera.org:8080/#/c/18953/7//COMMIT_MSG@12
PS7, Line 12: If the type is different, an error is reported.
nit: "If the column exists with a different type, after this patch an error will be reported."


http://gerrit.cloudera.org:8080/#/c/18953/7/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java:

http://gerrit.cloudera.org:8080/#/c/18953/7/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java@92
PS7, Line 92: // handle the case that ifNotExists_ is true
nit: this seems verbose and can be removed


http://gerrit.cloudera.org:8080/#/c/18953/7/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java@95
PS7, Line 95: type not match
Can we mention the types? E.g. already exists in type BIGINT


http://gerrit.cloudera.org:8080/#/c/18953/7/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java@98
PS7, Line 98: remove the column to be added which is existed.
nit: reword to "remove the existing column"


http://gerrit.cloudera.org:8080/#/c/18953/7/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java@99
PS7, Line 99:         iterator.remove();
If the column already exists, I think we don't need the following tests and we can add "continue" here.


http://gerrit.cloudera.org:8080/#/c/18953/7/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/18953/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@472
PS7, Line 472:     AnalyzesOk("alter table functional.alltypes add if not exists columns (int_col int)");
This duplicates line 469



-- 
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: 7
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 27 Sep 2022 13:21:16 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11565: Support IF NOT EXISTS in alter table add columns for kudu table

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins 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 table
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/11500/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 8
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 30 Sep 2022 11:16:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11565: For alter table, add column operation is optimized

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18953 )

Change subject: IMPALA-11565: For alter table, add column operation is optimized
......................................................................


Patch Set 3: Verified+1


-- 
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: 3
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Comment-Date: Tue, 13 Sep 2022 13:42:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11565: Support IF NOT EXISTS in alter table add columns for kudu/iceberg table

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.

lipenglin@sensorsdata.cn 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 12:

(3 comments)

Thanks for working on this! I left some minor comments here.

http://gerrit.cloudera.org:8080/#/c/18953/12/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/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1343
PS12, Line 1343: params.getAdd_cols_params() != null
               :               && params.getAdd_cols_params().getColumnsSize() != 0
A lot of similar logic code, can extract into a method?


http://gerrit.cloudera.org:8080/#/c/18953/12/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/12/testdata/workloads/functional-query/queries/QueryTest/iceberg-alter.test@439
PS12, Line 439: ---- QUERY
What happens when we add column for the same name, but of a different type, if they already exist?


http://gerrit.cloudera.org:8080/#/c/18953/12/testdata/workloads/functional-query/queries/QueryTest/iceberg-alter.test@442
PS12, Line 442: alter table ice_alter_cols add if not exists columns (a bigint, d bigint)
nit: The case of sql should be as consistent as possible with the rest of the code in the current file.



-- 
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: 12
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 27 Dec 2022 03:17:22 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11565: Support IF NOT EXISTS in alter table add columns for kudu/iceberg table

Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.

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 <xi...@163.com>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Mar 2023 01:01:04 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11565: Support IF NOT EXISTS in alter table add columns for kudu/iceberg table

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins 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 12:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/11975/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 12
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 06 Dec 2022 11:09:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11565: For alter table, add column operation is optimized

Posted by "Baike Xia (Code Review)" <ge...@cloudera.org>.

Baike Xia has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/18953 )

Change subject: IMPALA-11565: For alter table, add column operation is optimized
......................................................................

IMPALA-11565: For alter table, add column operation is optimized

For alter table, add if not exists column operation,
if the columns already exist and are of the same type,
no operation is performed;
If the type is different, an error is reported.

Change-Id: I82590e5372e881f2e81d4ed3dd0d32a2d3ddb517
---
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
4 files changed, 40 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/18953/5
-- 
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: newpatchset
Gerrit-Change-Id: I82590e5372e881f2e81d4ed3dd0d32a2d3ddb517
Gerrit-Change-Number: 18953
Gerrit-PatchSet: 5
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>

[Impala-ASF-CR] IMPALA-11565: For alter table, add column operation is optimized

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18953 )

Change subject: IMPALA-11565: For alter table, add column operation is optimized
......................................................................


Patch Set 7: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/8610/


-- 
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: 7
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Comment-Date: Mon, 26 Sep 2022 14:27:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11565: For alter table, add column operation is optimized

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18953 )

Change subject: IMPALA-11565: For alter table, add column operation is optimized
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/18953/4/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java:

http://gerrit.cloudera.org:8080/#/c/18953/4/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java@91
PS4, Line 91:         
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/18953/4/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java@97
PS4, Line 97:         
line has trailing whitespace



-- 
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: 4
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Comment-Date: Mon, 26 Sep 2022 07:47:00 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11565: For alter table, add column operation is optimized

Posted by "Jian Zhang (Code Review)" <ge...@cloudera.org>.

Jian Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18953 )

Change subject: IMPALA-11565: For alter table, add column operation is optimized
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/18953/7/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java:

http://gerrit.cloudera.org:8080/#/c/18953/7/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java@93
PS7, Line 93:         if (!col.getType().equals(c.getType())) {
IMHO, users are expected the query to get successfully executed when they add the `if not exists` clause in the `alter table` statements.

It's better to remove this validation, just ignore the duplicated columns, and support `alter table ... add column if not exists ...` for HDFS/Kudu tables.



-- 
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: 7
Gerrit-Owner: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Baike Xia <xi...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jian Zhang <zj...@gmail.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 28 Sep 2022 03:16:17 +0000
Gerrit-HasComments: Yes