Matthew Jacobs has posted comments on this change.

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

Patch Set 5:

File fe/src/main/java/org/apache/impala/catalog/

PS4, Line 175: }
> Done
While it does seem related, does it really mean the same thing?

Either way, we have to be sure we're handling it correctly everywhere. E.g. 
does  Table.getClusteringColumns(), getNonClusteringColumns(), work as expected?
They also get stored in the thrift TTable differently, see

      // Clustering columns come first.
      if (i < numClusteringCols_) {
      } else {

Further, Table.getNumClusteringCols() exposes this and it is used in a lot of 

  public boolean hasClusteringColsOnly() {
    Table table = getTable();
    if (!(table instanceof HdfsTable) || table.getNumClusteringCols() == 0) 
return false;

There are a ton of other places to check.

PS4, Line 226:   private void 
loadDistributeByParams(org.apache.kudu.client.KuduTable kuduTable) {
             :     Preconditions
> cols is a reference to msTable cols. We clear them here and reload them fro
I see. yes, a brief comment would be helpful.
File fe/src/main/java/org/apache/impala/service/

PS5, Line 1484:         // Add the table to the catalog cache
              :         Table newTbl = catalog_.addTable(newTable.getDbName(), 
              :         addTableToCatalogUpdate(newTbl, response.result);
If something here throws (e.g. addTable() looks like it could throw), we 
probably need to roll back the hiveClient.createTable() call, right?
File fe/src/main/java/org/apache/impala/service/

PS4, Line 135:           tableOpts.setRangePartitionColumns(
             :               distParam.getBy_range_param().getColumns());
> Unfortunately it is. I spoke to Dan (from Kudu team) about it. If the user 
Thanks, I followed up with an e-mail to Dan to make sure I understand the Kudu 

To view, visit
To unsubscribe, visit

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

Reply via email to