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

Posted to reviews@impala.apache.org by "Fredy Wijaya (Code Review)" <ge...@cloudera.org> on 2019/01/08 21:07:16 UTC

[Impala-ASF-CR] IMPALA-7832: Add support for ALTER TABLE ADD IF NOT EXISTS COLUMNS

Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12181


Change subject: IMPALA-7832: Add support for ALTER TABLE ADD IF NOT EXISTS COLUMNS
......................................................................

IMPALA-7832: Add support for ALTER TABLE ADD IF NOT EXISTS COLUMNS

This patch adds IF NOT EXISTS support in ALTER TABLE ADD COLUMNS. If IF
NOT EXISTS is specified and a column already exists with this name, no
error is thrown.

Syntax:
ALTER TABLE tbl ADD IF NOT EXISTS COLUMNS (i int, j int)

Testing:
- Added new FE tests
- Ran all FE tests
- Updated E2E DDL tests
- Ran all E2E DDL tests

Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9
---
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
R fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java
C fe/src/main/java/org/apache/impala/analysis/AlterTableReplaceColsStmt.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M testdata/workloads/functional-query/queries/QueryTest/alter-table.test
8 files changed, 230 insertions(+), 156 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/12181/1
-- 
To view, visit http://gerrit.cloudera.org:8080/12181
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9
Gerrit-Change-Number: 12181
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>

[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)

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

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

Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
......................................................................


Patch Set 9:

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


-- 
To view, visit http://gerrit.cloudera.org:8080/12181
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9
Gerrit-Change-Number: 12181
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Jan 2019 23:24:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)

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

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

Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1758/ : 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/12181
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9
Gerrit-Change-Number: 12181
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 23:23:21 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.

Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12181 )

Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
......................................................................


Patch Set 8:

Paul, Bharath gave a +1, can you take another look at it?


-- 
To view, visit http://gerrit.cloudera.org:8080/12181
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9
Gerrit-Change-Number: 12181
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Jan 2019 21:39:37 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.

Fredy Wijaya has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/12181 )

Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
......................................................................

IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)

This patch adds IF NOT EXISTS support in ALTER TABLE ADD COLUMN and
ALTER TABLE ADD COLUMNS. If IF NOT EXISTS is specified and a column
already exists with this name, no error is thrown.

Syntax:
ALTER TABLE tbl ADD COLUMN [IF NOT EXISTS] i int
ALTER TABLE tbl ADD [IF NOT EXISTS] COLUMNS (i int, j int)

Testing:
- Added new FE tests
- Ran all FE tests
- Updated E2E DDL tests
- Ran all E2E DDL tests

Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9
---
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
R fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java
C fe/src/main/java/org/apache/impala/analysis/AlterTableReplaceColsStmt.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M testdata/workloads/functional-query/queries/QueryTest/alter-table.test
9 files changed, 386 insertions(+), 179 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/12181/4
-- 
To view, visit http://gerrit.cloudera.org:8080/12181
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9
Gerrit-Change-Number: 12181
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)

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

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

Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
......................................................................


Patch Set 9: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/12181
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9
Gerrit-Change-Number: 12181
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Jan 2019 03:14:33 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.

Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12181 )

Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
......................................................................


Patch Set 7: Code-Review+1

(5 comments)

Nice test coverage. Paul, looks like you had some comments about refactoring. Can you take the final pass?

http://gerrit.cloudera.org:8080/#/c/12181/7/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/12181/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@612
PS7, Line 612: Column
nit: Column(s) have.., here and below.


http://gerrit.cloudera.org:8080/#/c/12181/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2046
PS7, Line 2046: nonExistingColumns
nit: I think colsToAdd will be more clear. Please ignore if you disagree.


http://gerrit.cloudera.org:8080/#/c/12181/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2049
PS7, Line 2049: if (ifNotExists && col != null) {
              :         continue;
              :       }
single line and else if below?


http://gerrit.cloudera.org:8080/#/c/12181/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2053
PS7, Line 2053: AnalysisException
We are past analysis at this point. Throw a CatalogException? Also, include the tbl name?


http://gerrit.cloudera.org:8080/#/c/12181/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2057
PS7, Line 2057: and ifNotExists is true, do nothing
we don't use it anymore, update?



-- 
To view, visit http://gerrit.cloudera.org:8080/12181
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9
Gerrit-Change-Number: 12181
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Jan 2019 19:41:34 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12181 )

Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
......................................................................

Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/12181/3/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

http://gerrit.cloudera.org:8080/#/c/12181/3/fe/src/main/cup/sql-parser.cup@1097
PS3, Line 1097: KW_ALTER KW_TABLE table_name:table KW_ADD KW_COLUMN if_not_exists_val:if_not_exists
> Is there anything in particular that you have in mind to be pulled out into
Each statement here starts with ALTER TABLE <table-name>. In parser-speak:

alter_tbl_stmt ::=
KW_ALTER KW_TABLE table_name:table alter_table_tail

alter_table_tail ::=
KW_ADD KW_COLUMN ...
| KW_ADD ...
| KW_DROP ...

Would need to pass the column name around somehow, which is probably why we have the current structure.

Not a huge issue, we can always clean this up later.

http://gerrit.cloudera.org:8080/#/c/12181/8/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/12181/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2048
PS8, Line 2048: Column col = tbl.getColumn(column.getColumnName());
What is the concurrency model? Suppose I start 100 threads each adding the same columns simultaneously. Is there a race condition between the check here and the actual add below?

Also, suppose I add 50 threads which remove the same columns. Is there a race condition where we don't add a column because it occurs in our cache, though it has actually been removed from the table in HMS?

We can't fix this issue, really, because there is no global locking. Still, we should think about expected behavior.

http://gerrit.cloudera.org:8080/#/c/12181/8/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/12181/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@328
PS8, Line 328: AnalyzesOk("alter table functional.alltypes add column if not exists INT_COL int");
Just a side note: While this kind of test is better than no test, it is rather limited. Doing absolutely nothing in analyze() will allow this test to pass.

Would be better to do a sampling of deeper tests. That is, get back the statement and probe to ensure that the table name was set and resolved. That the column names and types were set.

Such checks are implicitly done later, but catching bugs is easier of we verify the data structures at each step.

Adding deep tests is a bit more work, so not required here as we've not done this in the past. Offering it as something to consider in the future. (I've started adding such tests for the analyzer, etc.)

--
To view, visit http://gerrit.cloudera.org:8080/12181
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9
Gerrit-Change-Number: 12181
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Jan 2019 23:01:00 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)

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

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

Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
......................................................................


Patch Set 9: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/12181
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9
Gerrit-Change-Number: 12181
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Jan 2019 23:24:04 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)

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

Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12181 )

Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
......................................................................

IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)

This patch adds IF NOT EXISTS support in ALTER TABLE ADD COLUMN and
ALTER TABLE ADD COLUMNS. If IF NOT EXISTS is specified and a column
already exists with this name, no error is thrown. If IF NOT EXISTS
is specified for multiple columns and a column already exists, no
error is thrown and a new column that does not exist will be added.

Syntax:
ALTER TABLE tbl ADD COLUMN [IF NOT EXISTS] i int
ALTER TABLE tbl ADD [IF NOT EXISTS] COLUMNS (i int, j int)

Testing:
- Added new FE tests
- Ran all FE tests
- Updated E2E DDL tests
- Ran all E2E DDL tests

Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9
Reviewed-on: http://gerrit.cloudera.org:8080/12181
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
R fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java
C fe/src/main/java/org/apache/impala/analysis/AlterTableReplaceColsStmt.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M testdata/workloads/functional-query/queries/QueryTest/alter-table.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_insert.test
11 files changed, 398 insertions(+), 182 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/12181
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9
Gerrit-Change-Number: 12181
Gerrit-PatchSet: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.

Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12181 )

Change subject: IMPALA-7832: Support for IF NOT EXISTS in ALTER TABLE ADD COLUMN(S)
......................................................................


Patch Set 4:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/12181/3/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

http://gerrit.cloudera.org:8080/#/c/12181/3/fe/src/main/cup/sql-parser.cup@1097
PS3, Line 1097:   KW_ALTER KW_TABLE table_name:table KW_ADD KW_COLUMN if_not_exists_val:if_not_exists
> I wonder if this is asking to be refactored to pull out the increasingly-de
Is there anything in particular that you have in mind to be pulled out into the common bits?


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

http://gerrit.cloudera.org:8080/#/c/12181/3/fe/src/main/java/org/apache/impala/analysis/AlterTableReplaceColsStmt.java@83
PS3, Line 83:       if (!colNames.add(colName)) {
> Should we be adding lower-case names? If we do a SELECT *, shouldn't we giv
colNames is only used for checking duplicate column names. I believe we preserve the case for the column names passed to HMS.


http://gerrit.cloudera.org:8080/#/c/12181/3/fe/src/main/java/org/apache/impala/analysis/AlterTableReplaceColsStmt.java@87
PS3, Line 87:   }
> A bit beyond the scope of your change, but since you are refactoring...
Actually I just realized Kudu does not support ALTER TABLE REPLACE COLUMNS, so this whole logic can be removed. The original code combined the two statements into one which was confusing.


http://gerrit.cloudera.org:8080/#/c/12181/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/12181/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2042
PS3, Line 2042:     if (ifNotExists) {
> Shouldn't this check be done at analysis time so that execution is always g
I was initially thinking of doing it in the analysis. However, looking at other code in this file with "IF EXISTS" or "IF NOT EXISTS", it seems we handle "IF EXISTS" and "IF NOT EXISTS" in this file. I do agree, that may not be ideal.


http://gerrit.cloudera.org:8080/#/c/12181/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2075
PS3, Line 2075:       msTbl.getParameters().put(sortByKey, alteredColumns);
> Again, seems this should be done at analysis time so that exec just runs th
Similar argument as above.


http://gerrit.cloudera.org:8080/#/c/12181/3/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/12181/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@326
PS3, Line 326:     AnalyzesOk("alter table functional.alltypes add column NEW_COL int");
> Test with various case combinations? INT_COL, etc.
Done


http://gerrit.cloudera.org:8080/#/c/12181/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@338
PS3, Line 338:     AnalysisError("alter table functional.alltypes add column if not exists year int",
> What are the rules for which names are valid? Do we check the name, or leav
We should rely on the host system to do the check. I don't think it's a good idea to mimic the rules of the target system since if the host rules change, we need to update our rules as well.


http://gerrit.cloudera.org:8080/#/c/12181/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@368
PS3, Line 368:         "ALTER TABLE ADD COLUMNS not currently supported on HBase tables.");
> Do we have a test for ...(foo int, bar string, foo int)? That is, the dupli
yeah it's in L383


http://gerrit.cloudera.org:8080/#/c/12181/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@420
PS3, Line 420: 
> Is the column validation code in common with the above case for Add Column?
Yeah ColumnDef.anayze() is the code that does column validation. Unfortunately there's no specific test for that class. I can create a follow up JIRA for ColumnDef test coverage.


http://gerrit.cloudera.org:8080/#/c/12181/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@459
PS3, Line 459:         "Kudu tables do not support complex types: c STRUCT<f1:INT>");
> Overall, nice test case coverage. Thanks!
Thanks! I also added more missing test cases especially for Kudu tables.



-- 
To view, visit http://gerrit.cloudera.org:8080/12181
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60ed22c8a8eefa10e94ad3dedf32fe67c16642d9
Gerrit-Change-Number: 12181
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 22:51:27 +0000
Gerrit-HasComments: Yes