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 10: (4 comments) Thanks Dimitris for the review! http://gerrit.cloudera.org:8080/#/c/8820/9/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java: http://gerrit.cloudera.org:8080/#/c/8820/9/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@124 PS9, Line 124: if (!Table.isExternalTable(table_.getMetaStoreTable()) && !setsToExternal) { : AnalysisUtils.throwI > nit: check if they fit in a single line (I could be wrong) Indeed, it fits :) Done http://gerrit.cloudera.org:8080/#/c/8820/5/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/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@315 PS5, Line 315: throw new AnalysisExce > In principal yes, in this case no. The name of this function doesn't reveal renamed to analyzeManagedKuduTableName(). Hope that is a bit more suitable here :) http://gerrit.cloudera.org:8080/#/c/8820/9/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/9/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@67 PS9, Line 67: public CreateTableStmt(TableDef tableDef) { : Preconditions.checkNotNull(tableDef); > Since all the tblproperties live in TableDef, I believe it makes more sense Sure, done. http://gerrit.cloudera.org:8080/#/c/8820/9/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/8820/9/tests/query_test/test_kudu.py@327 PS9, Line 327: try: : cursor.execute("ALTER TABLE %s.foo SET TBLPROPERTIES('kudu.table_name'='blah')" : % (unique_database)) : except Exception as e: : expected_error = "AnalysisException: Not allowed to set 'kudu.table_name' " + \ : "manually for managed Kudu tables" : assert expected_error in str(e) > That should be an analysis test. There is also a test for this in AnalyzeDDLTest.java. Do you think this one should be dropped then? -- 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: 10 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: Dimitris Tsirogiannis <dtsirogian...@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: Mon, 08 Jan 2018 10:39:50 +0000 Gerrit-HasComments: Yes
