Matthew Jacobs has posted comments on this change. Change subject: Simplify creating external Kudu tables and add DROP DATABASE CASCADE ......................................................................
Patch Set 7: (4 comments) 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 237: fileFormat_ != THdfsFileFormat.KUDU > Same idea as above. I think of these as Kudu logic leakages. Ideally all th But we don't check this case I'm talking about later... That would mean throwing if columnDefs_ is empty in the internal branch. Then we'd handle both cases in analyzeKudu() and this check here wouldn't be necessary, so you could remove it and further reduce kudu logic leakages :) Line 308: tblProperties_ > I chatted with Alex a bit. I'm going to try setting the default reset() beh Sure. I just pointed it out because it's inconsistent with the interface. Throwing seems fine to me. Line 335: keyCols > That's part 2, updates for managed tables. There shouldn't be any regressio Ok, I'll look for that there then. 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 > This isn't intended to be thread safe. I can add a comment if you like. No need to add a comment if no race is possible, but if it is (e.g. multiple test threads) then we should handle it to avoid adding flaky tests. Why bother storing this as __DB_NAME at all? Why not just have an instance variable for individual test cases? Then we avoid any concerns. -- 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
