Dimitris Tsirogiannis 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 9: (4 comments) 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) { nit: check if they fit in a single line (I could be wrong) 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: "Cannot create tab > I kind of disagree here: The underlying function is 5 lines long, and readi In principal yes, in this case no. The name of this function doesn't reveal that you're actually analyzing this tbl property (checking for its existence) and setting its value at the same time, so as a reader I have to check the definition of this function. So in my mind this is pure overhead and doesn't really help me understand the code. Feel free to keep it if you want though, it's not that important :) 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: // Kudu table name generated during analysis for managed Kudu tables : private String generatedKuduTableNameProperty_ = ""; Since all the tblproperties live in TableDef, I believe it makes more sense to put that generated field there as well. 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. -- 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: 9 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: Sat, 06 Jan 2018 01:02:08 +0000 Gerrit-HasComments: Yes
