Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/8820 )
Change subject: IMPALA-5654: Disallow setting Kudu table name in CREATE TABLE ...................................................................... Patch Set 3: (13 comments) http://gerrit.cloudera.org:8080/#/c/8820/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8820/2//COMMIT_MSG@9 PS2, Line 9: explici > explicitly setting Done http://gerrit.cloudera.org:8080/#/c/8820/2//COMMIT_MSG@10 PS2, Line 10: for man > This implies that the property can be still set in CREATE EXTERNAL TABLE st Because it can be set for external tables :) Well, I don't see what extra info is required other than disallowing setting one property for managed kudu tables and still allowing it for external kudu tables. Here, I think an example wouldn't add extra value either. http://gerrit.cloudera.org:8080/#/c/8820/2//COMMIT_MSG@14 PS2, Line 14: Providing the Kudu table name property when creating a managed Kudu > I would add a full example for the kind of CREATE TABLE statement that is n Done http://gerrit.cloudera.org:8080/#/c/8820/2/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java: http://gerrit.cloudera.org:8080/#/c/8820/2/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java@168 PS2, Line 168: if (isReAnalyzed()) createStmt_.setIsReAnalyzed(); : createStmt_.analyze(analyzer); : > nit: one line Done http://gerrit.cloudera.org:8080/#/c/8820/2/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/8820/2/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@341 PS2, Line 341: hence skipping > setManagedKuduTableName() ? Done http://gerrit.cloudera.org:8080/#/c/8820/2/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@342 PS2, Line 342: / > nit: Perhaps moving the condition before L297 would make the code more read Agree. Done http://gerrit.cloudera.org:8080/#/c/8820/2/fe/src/main/java/org/apache/impala/analysis/StatementBase.java File fe/src/main/java/org/apache/impala/analysis/StatementBase.java: http://gerrit.cloudera.org:8080/#/c/8820/2/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@39 PS2, Line 39: Set to True if statement is being re-analyzed. > Wording is a bit weird. Something like this might sound better: Done http://gerrit.cloudera.org:8080/#/c/8820/2/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@40 PS2, Line 40: isReAnalyzed > I think 'isReAnalyzed' would be more readable. Done http://gerrit.cloudera.org:8080/#/c/8820/2/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/8820/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@2205 PS2, Line 2205: Kudu num_tablet_replicas is specified in tblp > Comment is incorrect. Done http://gerrit.cloudera.org:8080/#/c/8820/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@2206 PS2, Line 2206: AnalyzesOk("create table tab (x int primary key) partition by hash (x) " + : "partitions 8 stored as kudu tblproperties ('kudu.num_tablet_replicas'='1'," + : "'kudu.master_addresses' = '127.0.0.1:8080, 127.0.0.1:8081')"); > You should add an AnalysisError() test case after this, to make sure that s Done http://gerrit.cloudera.org:8080/#/c/8820/2/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/8820/2/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test@358 PS2, Line 358: ImpalaRuntimeException: Error renaming Kudu table impala::$DATABASE.copy_of_tbl to kudu_tbl_to_alter > This looks a bit strange - are you sure that the name of the database will I'm not sure about changing ALTER TABLE as well. Let me find out what others think in the Jira. http://gerrit.cloudera.org:8080/#/c/8820/2/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test@358 PS2, Line 358: impala::$DATABASE.copy_of_tbl to kudu_tbl_to_alter > You should do something like: impala::$DATABASE.copy_of_tbl Good point. Anyway this was a generated DB name, so would have failed :D Done http://gerrit.cloudera.org:8080/#/c/8820/2/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/8820/2/tests/query_test/test_kudu.py@857 PS2, Line 857: def test_kudu_table_name_property(self, cursor): : # If an explicit Kudu table name is provided for a managed Kudu table : # as table property then an error is expected. : kudu_table = self.random_table_name() : props = "'kudu.table_name'='%s'" % kudu_table : try: : cursor.execute( : """CREATE TABLE some_table (c INT PRIMARY KEY) : PARTITION BY HASH (c) PARTITIONS 3 : STORED AS KUDU : TBLPROPERTIES ({props})""".format(props=props)) : assert False : except Exception as e: : assert "AnalysisException: Not allowed to set 'kudu.table_name' " + \ : "manually for managed Kudu tables" in str(e) > I think this test case should be covered in AnalyzeDDLTest.java instead. I added a test there, but I feel that this is also useful as the scope here is wider than analysis of the query. -- To view, visit http://gerrit.cloudera.org:8080/8820 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieca037498abf8f5fde67b77e824b720482cdbe6f Gerrit-Change-Number: 8820 Gerrit-PatchSet: 3 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Laszlo Gaal <laszlo.g...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 12 Dec 2017 14:15:38 +0000 Gerrit-HasComments: Yes
