Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables

Patch Set 1:


Ok, part 3. I have one more batch left...
File fe/src/main/java/com/cloudera/impala/catalog/

PS1, Line 222: 
was this moved somewhere else? Maybe I'll find it as I keep reading...

PS1, Line 238: // Update the HMS
             :     if (reuseMetadata) {
             :       try {
             :         client.alter_table(msTable_.getDbName(), 
msTable_.getTableName(), msTable_);
             :       } catch (TException e) {
             :         throw new TableLoadingException(e.getMessage());
             :       }
             :     }
Same deal, I'm not sure why reuseMetadata means update the HMS.
File fe/src/main/java/com/cloudera/impala/service/

PS1, Line 220: 
             :   private final static KuduCatalogOpExecutor kuduExecutor_
             :       = new KuduCatalogOpExecutor();
all static methods, why instantiate it?

PS1, Line 1122:  // The db == null case isn't handled. The only tables this 
should matter for are
              :     // Kudu tables. The expectation is Impala will always know 
about any Kudu tables
              :     // because Hive doesn't support Kudu yet. 
I've always had a problem with this comment :/ even after trimming it down I 
don't think it makes complete sense. This should mention briefly what this code 
_is handling_ before it talks about what it's not handling. Can you add 1 
sentence at the beginning?

PS1, Line 1124: When Hive supports Kudu the DDL delegates
              :     // can be removed. tracks the removal.
Can you remove these sentences and just leave IMPALA-3424 as a reference? We 
may not remove our kudu code even if/when Hive supports Kudu.

PS1, Line 1127: ,
nit: extra comma

PS1, Line 1150:         // The operation will be aborted if the Kudu table 
cannot be dropped. If for
              :         // some reason Kudu is permanently stuck in a 
non-functional state, the user is
              :         // expected to ALTER TABLE to either set the table to 
UNMANAGED or set the format
              :         // to something else.
JIRA? Even if we don't fix it we should have something public to point to 
regarding this behavior. Also to make sure the docs team knows to document it.

PS1, Line 1205: kuduExecutor_
static ref

PS1, Line 1406: if (KuduTable.isKuduTable(tbl)) {
              :       return createKuduTable(tbl, params.if_not_exists, 
              :           response);
              :     }
              :     return createTable(tbl, params.if_not_exists, 
params.getCache_op(), response);
              :   }
it's too bad we have to just branch like this and handle everything 
differently, but I can't think of anything better.

PS1, Line 1420:  createMetaStoreTable
I'd prefer to have this on the prev line and the parameter on the new line

Line 1475:    * creation of a managed Kudu table.
comment on response param

PS1, Line 1480: TDdlExecResponse response)
I think this just fits on the prev line

PS1, Line 1483: kuduExecutor_
static reference KuduCatalogOpExecutor, here elsewhere in this fn

PS1, Line 1509:       // Add the table to the catalog cache
              :       Table newTbl = catalog_.addTable(newTable.getDbName(), 
              :       addTableToCatalogUpdate(newTbl, response.result);
While I don't think these will throw, it might be worth wrapping all the logic 
after the Kudu create table in a try { } catch block that drops the kudu table. 
That'll future proof this a bit. Feel free to ignore though.
File fe/src/main/java/com/cloudera/impala/service/

PS1, Line 230: impaladCatalog_.getDefaultKuduMasterAddrs()
this is fine but a little weird, normally I'm all about removing extra state 
but in this case it might be better to keep this as a parameter on frontend and 
pass it in. Unless there's somewhere else besides the old impaladCatalog we can 
get it?
File fe/src/main/java/com/cloudera/impala/service/

PS1, Line 83: 
            :       int otherLogLevel, boolean allowAuthToLocal, String 
File fe/src/main/java/com/cloudera/impala/service/

PS1, Line 29: import org.apache.hadoop.hive.metastore.api.T
since this file references our Table class we should import ours and reference 
this one w/ namespace. that's just our convention.

PS1, Line 45: yet

PS1, Line 45: When Hive
            :  * functionality is available, that should be preferred to the 
functionality here.
            :  * tracks this.
remove text since we might not. JIRA can stay but is actually a different issue 

PS1, Line 65: com.cloudera.impala.catalog.Table.
shouldn't need to ref the full namespace

Line 149:    * Reads the schema from a Kudu table and populates 'msTbl' with an 
equivalent schema.
If an error occurs we may have partially modified the table and leave it in an 
inconsistent state.

PS1, Line 150: if unable to do so.
if any errors are encountered.

PS1, Line 155:       // Schemas for external tables are not specified by the 
user in DDL. Instead the
             :       // user provides a Kudu table name (or uses implicit Hive 
Metastore to Kudu mapping
             :       // rules), then the schema is imported from Kudu.
I'm not sure if this is adding much and I think it's a bit confusing/out of 

PS1, Line 162: // Start searching....

PS1, Line 166: if (!Strings.isNullOrEmpty(dbName) && 
!dbName.equalsIgnoreCase("default")) {
             :           // If it exists, the fully qualified name is preferred.
             :           candidateTableNames.add(dbName + "." + tableName);
             :         }
i don't think we should bother handling the default case specially. Can we just 
always handle it as dbname.tableName? ...and drop the 'candidate' below?
File fe/src/main/java/com/cloudera/impala/util/

PS1, Line 30:  * This class wraps an org.apache.kudu.client.KuduClient to 
transform exceptions into
            :  * ImpalaRuntimeExceptions. No additional functionality is 
provided. See the Kudu
            :  * documentation for information about the methods.
Let's think about whether or not we can avoid this. Obviously this will have to 
be updated when kudu client changes... blegh.
File fe/src/main/java/com/cloudera/impala/util/

PS1, Line 41: org.apache.kudu.
reference the ext Type and import ours

PS1, Line 191: com.cloudera.impala.catalog.

PS1, Line 191: isSupportedKeyType
is this their restriction?

PS1, Line 196:  and the user did
             :    * not provide an explicit Kudu table name
... a table, assuming a custom name was not provided.

PS1, Line 201: Catalog.isDefaultDb(metastoreDbName) ?
you know i'm voting for not special casing this...

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7b9d51b2720ab57649abdb7d5c710ea04ff50dc1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

Reply via email to