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

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


Patch Set 8:

(6 comments)

> Patch Set 8:
>
> (1 comment)
>
> > Patch Set 8: Verified-1
> >
> > Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5114/
>
> Some test failures need to fix: 
> https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/8384/
> Test Result (3 failures / +3)
> org.apache.impala.analysis.AnalyzeKuduDDLTest.TestCreateExternalKuduTable
> org.apache.impala.planner.PlannerTest.testHbase
> org.apache.impala.planner.PlannerTest.testHbaseNoKeyEstimate
>
> Hao, you can run these java tests locally according to 
> https://cwiki.apache.org/confluence/display/IMPALA/How+to+load%2C+run%2C+and+create+new+Impala+tests
> e.g. in $IMPALA_HOME:
> (pushd fe && mvn test -Dtest=AnalyzeKuduDDLTest#TestCreateExternalKuduTable)
> (pushd fe && mvn test -Dtest=PlannerTest#testHbase)
> (pushd fe && mvn test -Dtest=PlannerTest#testHbaseNoKeyEstimate)
>
> Looks like we broke something in HBase Tables. I'll also look into it 
> tomorrow.

Thanks Quanlong. I didn't do it as I don't have a suitable environment to set 
up Impala (which can takes hours to do), and I think this change is obvious 
enough to rely on the Jenkins output.

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
Good point! Updated.


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(
> Sorry, don't need to move these... Only Kudu tables are affected.
Ack


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
Done


http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@506
PS8, Line 506: tab (x int)
> Should not specify columns for creating external Kudu table.
Done


http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@507
PS8, Line 507: purges
> typo: purge
Done


http://gerrit.cloudera.org:8080/#/c/14397/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@508
PS8, Line 508: purges
> same here: purge
Done



--
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 <[email protected]>
Gerrit-Reviewer: Andrew Wong <[email protected]>
Gerrit-Reviewer: Grant Henke <[email protected]>
Gerrit-Reviewer: Greg Solovyev <[email protected]>
Gerrit-Reviewer: Hao Hao <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Quanlong Huang <[email protected]>
Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]>
Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]>
Gerrit-Comment-Date: Mon, 21 Oct 2019 18:35:54 +0000
Gerrit-HasComments: Yes

Reply via email to