impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Casey Ching (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) Simplify creating external Kudu tables and add DROP DATABASE CASCADE
Date Tue, 26 Apr 2016 03:14:42 GMT
Casey Ching has posted comments on this change.

Change subject: Simplify creating external Kudu tables and add DROP DATABASE CASCADE

Patch Set 7:

File fe/src/main/java/com/cloudera/impala/analysis/

Line 1: package com.cloudera.impala.analysis;
> legal header
File fe/src/main/java/com/cloudera/impala/analysis/

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

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

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

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
File fe/src/main/java/com/cloudera/impala/catalog/delegates/

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 

          "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"

Line 194: com.cloudera.impala.catalog.
> I think you can remove the namespace as it's clear from l197 what's going o

Line 204:;
> nit: just put this directly on l205 and save a line

Line 213:  
> nit: space
File fe/src/main/java/com/cloudera/impala/service/

Line 1126: s
> scenario

Line 1131: If that's assumption isn't
> This doesn't make sense. that's -> that?

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
         :     // 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
         :     // would require a DdlDelegate. Another scenarios is when the user creates
         :     // that reqire a DdlDelegate in Impala, then drops the database in Hive. Ideally
         :     // user visible result of DROP DATABASE CASCADE would be the same in any case.
         :     // best solution is probably to move the DdlDelegate functionality into Hive.
         :     // now Impala will assume that any table not in its cache also doesn't require
         :     // use of a DdlDelegate. If that's assumption isn't correct, the database will
         :     // 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

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 

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
File testdata/workloads/functional-query/queries/QueryTest/kudu-show-create.test:

> what about distribution parameters?
That's not related to this change so I'll do it later. Mike already filed a jira for this
File tests/common/

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.
File tests/query_test/

Line 110: a
> an external

Line 145: cursor.execute("""
        :             CREATE EXTERNAL TABLE %s
        :             STORED AS KUDU
        :             TBLPROPERTIES('kudu.table_name' = '%s')""" % (
        :                 impala_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

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic141102818b6dad3016181b179a14024d0ff709d
Gerrit-PatchSet: 7
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Casey Ching <>
Gerrit-Reviewer: Casey Ching <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

View raw message