date:20191018

[Impala-ASF-CR] IMPALA-9030: Handle translated external Kudu tables

2019-10-18 Thread Quanlong Huang (Code Review)

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

Change subject: IMPALA-9030: Handle translated external Kudu tables
..

Patch Set 8:

(3 comments)

I think HDFS and HBase tables also have this issue. For example, creating a
non-transactional HDFS table will finally create an external table. Then in
DROP TABLE we won't drop HDFS files.

However, let's keep this fix simple and focus on kudu first.

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

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@341
PS8, Line 341: // External table cannot have 'external.table.purge'
property set, which is considered
: // equivalent to managed table.
: if (Boolean.parseBoolean(
:
getTblProperties().get(KuduTable.TBL_PROP_EXTERNAL_TABLE_PURGE))) {
: throw new AnalysisException(String.format("Table property
'%s' cannot be set to " +
: "true with an external Kudu table.",
KuduTable.TBL_PROP_EXTERNAL_TABLE_PURGE));
: }
I think manually changing 'external.table.purge' should not be allowed. We
should also check this in AlterTableSetTblProperties#analyze(). Please also add
a test to cover it.

Fortunately, CREATE TABLE LIKE statement don't allow setting tblproperties and
won't copy them. I think no other statements will modify the tblproperties.

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@140
PS8, Line 140: public static boolean isSynchronizedTable(
Could you move these three functions into Table.java? I feel like it's not just
a kudu issue. Tables on HDFS and HBase may also need these. However, let's keep
this patch simple to just focus on Kudu. We can deal with HDFS, HBase issues in
other patches.

http://gerrit.cloudera.org:8080/#/c/14397/8/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/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@505
PS8, Line 505: // Cannot specify the number of replicas for external Kudu
tables
Comment looks incorrect here. Maybe: External Kudu table is not allowed to set
table property 'external.table.purges' to true.

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36
Gerrit-Change-Number: 14397
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Quanlong Huang
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Sat, 19 Oct 2019 06:21:38 +
Gerrit-HasComments: Yes

69 matches

Mail list logo