Casey Ching has posted comments on this change. Change subject: Simplify creating external Kudu tables and add DROP DATABASE CASCADE ......................................................................
Patch Set 7: (27 comments) http://gerrit.cloudera.org:8080/#/c/2617/7/fe/src/main/java/com/cloudera/impala/analysis/AnalysisUtil.java File fe/src/main/java/com/cloudera/impala/analysis/AnalysisUtil.java: Line 1: package com.cloudera.impala.analysis; > legal header Done http://gerrit.cloudera.org:8080/#/c/2617/7/fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java File fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java: Line 151: getRowFormat > does this need to be public? No, there were also a few others that didn't need that either. Removed "pubic". Line 152: rowFormat_ != null ? rowFormat_ : : fileFormat_ == THdfsFileFormat.TEXT ? RowFormat.DEFAULT_ROW_FORMAT : null; > can you change the first ternary to if/else for readability, and in case it Done Line 224: fileFormat_ != THdfsFileFormat.KUDU && : KuduTable.KUDU_STORAGE_HANDLER.equals( : tblProperties_.get(KuduTable.KEY_STORAGE_HANDLER)) > This allows the Kudu storage handler to be set if the fileFormat_ is KUDU. That's fine here. The check is more about making sure that other file formats don't use the Kudu storage handler, most importantly text. The check when fileFormat_ == KUDU happens later. Line 237: fileFormat_ != THdfsFileFormat.KUDU > Following up on my comment from an earlier revision. If we don't check that Same idea as above. I think of these as Kudu logic leakages. Ideally all this stuff would be in analyzeKudu() but that's not really possible. Instead the leakages are minimized. This check is more about non-Kudu tables. Line 298: throw new AnalysisException(String.format( : "ROW FORMAT cannot be specified for file format %s.", fileFormat_)); > can you check if ff != TEXT sooner to throw this, then same on the indentat Done Line 308: tblProperties_ > Re-analyze still seems broken... I chatted with Alex a bit. I'm going to try setting the default reset() behavior to throw an exception. Apparently only a few stmts need that ability. Putting the burden on all stmts to support reset() when its not really needed seems bad. I'll give that a try in separate patch. Line 331: columnDefs_ > In the case of reanalysis (reset() -> analysis()) we'll add duplicates of t Same as above about reset() not needed. Line 335: keyCols > I think we might want some additional analysis on the keys, e.g., are the k That's part 2, updates for managed tables. There shouldn't be any regression around that here though. http://gerrit.cloudera.org:8080/#/c/2617/7/fe/src/main/java/com/cloudera/impala/catalog/delegates/KuduDdlDelegate.java File fe/src/main/java/com/cloudera/impala/catalog/delegates/KuduDdlDelegate.java: Line 160: Preconditions.checkState( : cols.get(0).getName().equals(REQUIRED_THRIFT_COLUMN_DEF.getColName())); > I didn't think we prevented users from creating an external table with colu It's already there AnalysisUtil.throwIfNotNullOrNotEmpty(columnDefs_, "Columns cannot be specified with an external Kudu table."); in analyzeKudu() Line 164: kuduTable.getSchema().getColumns() > do we know if this comes back in a consistent order? Given we flatten the n Looks like they are in "index order" https://github.com/cloudera/kudu/blob/master/java/kudu-client/src/main/java/org/kududb/Schema.java#L143 Line 194: com.cloudera.impala.catalog. > I think you can remove the namespace as it's clear from l197 what's going o Done Line 204: csb.build(); > nit: just put this directly on l205 and save a line Done Line 213: > nit: space Done http://gerrit.cloudera.org:8080/#/c/2617/7/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: Line 1126: s > scenario Done Line 1131: If that's assumption isn't > This doesn't make sense. that's -> that? Done Line 1121: // The use of DdlDelegates combined with the fact that the catalog is disconnected : // from the source of truth, the Hive metastore, leads to some complications here. : // If 'db' is null, that should mean either the database doesn't really exist or the : // database was created outside of Impala and the user didn't run "invalidate : // metadata". If 'db' is not null, there still may be tables in the metastore that : // would require a DdlDelegate. Another scenarios is when the user creates tables : // that reqire a DdlDelegate in Impala, then drops the database in Hive. Ideally the : // user visible result of DROP DATABASE CASCADE would be the same in any case. The : // best solution is probably to move the DdlDelegate functionality into Hive. For : // now Impala will assume that any table not in its cache also doesn't require the : // use of a DdlDelegate. If that's assumption isn't correct, the database will still : // be dropped in the metastore but the underlying data would remain. > this is getting big, how about creating a JIRA to track this Hive/Impala in Filed https://issues.cloudera.org/browse/IMPALA-3424 Line 1132: // be dropped in the metastore but the underlying data would remain. > Can you add that users can be sure to avoid this by calling refresh before Done Line 1159: the user is expected to ALTER TABLE to either set the : // table to UNMANAGED or set the format to one that doesn't require a delegate. > we should track this somehow so we can eventually get it in documentation. Ya I was going to mention this to John. I just started a list. Line 1541: setDistributeParams(distributeBy) > This is only relevant to Kudu, so it'd be nice if we could have the KuduDdl This entire "ddl delegate" thing seems premature considering there is only one ddl delegate. I'd rather just remove it but I don't want to do that now. Line 1540: DdlDelegate ddlDelegate = createDdlDelegate(newTable) : .setDistributeParams(distributeBy); : ddlDelegate.createTable(); > Marcel had said in the sync-up about doing this only when you had a real de It's not clear he really understood how this was used. I'm going to wait on that. Line 1558: ddlDelegate.dropTable(); > This would be affected as well Same http://gerrit.cloudera.org:8080/#/c/2617/7/testdata/workloads/functional-query/queries/QueryTest/kudu-show-create.test File testdata/workloads/functional-query/queries/QueryTest/kudu-show-create.test: Line 10: STORED AS KUDU > what about distribution parameters? That's not related to this change so I'll do it later. Mike already filed a jira for this https://issues.cloudera.org/browse/IMPALA-2634 http://gerrit.cloudera.org:8080/#/c/2617/7/tests/common/kudu_test_suite.py File tests/common/kudu_test_suite.py: Line 52: def get_db_name(cls): : if not cls.__DB_NAME: : cls.__DB_NAME = \ : choice(ascii_lowercase) + "".join(sample(ascii_lowercase + digits, 5)) : return cls.__DB_NAME > what about handling the race? This isn't intended to be thread safe. I can add a comment if you like. http://gerrit.cloudera.org:8080/#/c/2617/7/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: Line 110: a > an external Done Line 145: cursor.execute(""" : CREATE EXTERNAL TABLE %s : STORED AS KUDU : TBLPROPERTIES('kudu.table_name' = '%s')""" % ( : impala_table_name, preferred_kudu_table.name)) > Hm, I don't think we should allow this behavior because there's no way to i I'm not sure I follow, hasn't the user given an explicit name here? Why would there be any confusion? Line 183: cursor > The same db is already the default db for this db, right? Can you add a com Added. I doubt that's the only place that relies on this behavior but I didn't check. It'll be a little strange if this is the only place that mentions this. It'll also be noisy to sprinkle these messages around. -- To view, visit http://gerrit.cloudera.org:8080/2617 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic141102818b6dad3016181b179a14024d0ff709d Gerrit-PatchSet: 7 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Casey Ching <[email protected]> Gerrit-Reviewer: Casey Ching <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
