Dimitris Tsirogiannis has posted comments on this change.

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

Patch Set 5:


File bin/start-impala-cluster.py:

PS5, Line 73: parser.add_option("--kudu_masters", default="",
> Could we import the default value from tests.common instead of hardcoding i
Good point. Done

File fe/src/main/java/org/apache/impala/analysis/DistributeParam.java:

Line 129:           if 
> you want to be able to assign the split value to the column type with just 

File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

PS4, Line 226:   private void 
loadDistributeByParams(org.apache.kudu.client.KuduTable kuduTable) {
             :     Preconditions
> yes, please leave a comment that you're resetting the mstable cols if there

PS4, Line 226:   private void 
loadDistributeByParams(org.apache.kudu.client.KuduTable kuduTable) {
             :     Preconditions
> I see. yes, a brief comment would be helpful.

File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

Line 157:   public void load(boolean reuseMetadata /* not used */, 
IMetaStoreClient msClient,
> rename reuseMetadata to dummy
Good point. Done

PS5, Line 193:       
             :           StatsSetupConst.TRUE);
> This also needs to be stripped out later for SHOW CREATE TABLE.

Line 217:       cols.add(new FieldSchema(colName, type.toSql().toLowerCase(), 
> this also updates msTable_
Added a comment to clarify it. Currently, we need to keep the schema in HMS 
consistent with the one in Kudu in order to use the column statistics. Once we 
move away from this and Kudu provides us with proper statistics API we will get 
rid of this annoying dance around msTable_.

Line 295:         TDistributeByRangeParam rangeParam = 
> checkstate(isset...)

File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

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 pr
Actually, addTable should not be declared to throw because none of its 
operations throw an exception. addTableToCatalogUpdate also doesn't throw, so I 
believe it's safe to assume that we don't need to rollback the hms change here. 
Modified the addTable() function in CatalogServiceCatalog.

File tests/common/kudu_test_suite.py:

PS5, Line 38: @SkipIf.kudu_not_supported
> This is extremely risky due to bugs in our version of old pytest. See IMPAL
Done. Let me know is that's ok now.

File tests/custom_cluster/test_kudu.py:

PS5, Line 1: # Copyright (c) 2016 Cloudera, Inc. All rights reserved.
> Remove Cloudera copyright and make sure the license text is correct otherwi

File tests/metadata/test_ddl.py:

PS5, Line 220: 
> Why remove this skip?
Hm, that wasn't intentional. I put it back.

PS5, Line 221: 
> Is this removal safe?
Same here.

PS5, Line 221:     self.expected_exceptions = 2
> I can't see where this is used; delete?

File tests/query_test/test_kudu.py:

PS5, Line 87:         print("Describe formatted output:")
            :         pprint(table_desc)
> Use LOG.info()

To view, visit http://gerrit.cloudera.org:8080/4414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7b9d51b2720ab57649abdb7d5c710ea04ff50dc1
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dtsirogian...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to