Matthew Jacobs has posted comments on this change. Change subject: Simplify creating external Kudu tables and add DROP DATABASE CASCADE ......................................................................
Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/2617/3//COMMIT_MSG Commit Message: Line 23: KUDU as a file format > Kudu isn't supported at all in Hive yet. As far as I know there's no way to Yeah, I'm wondering if you've chatted with them about what they're planning? It'd be good for us to stay sync'd with them. http://gerrit.cloudera.org:8080/#/c/2617/3/fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java File fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java: Line 298: rowFormat_ = RowFormat.DEFAULT_ROW_FORMAT; > Ah I hadn't considered reset(). Keeping null for the row format seems bette Yeah, keeping it null seems fine as long as whatever code that reads it later can handle it. Even though it's optional it may have been expected to be there- can you check? http://gerrit.cloudera.org:8080/#/c/2617/3/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: Line 1129: For : // now Impala will assume that any table not in its cache also doesn't require the : // use of a DdlDelegate. > Yes the Kudu tables would remain but the database metadata in Hive would be Have you chatted with the Hive team about their kudu integration plans? We should make sure that these can be addressed later. This seems like it'll be a very important thing to document. http://gerrit.cloudera.org:8080/#/c/2617/3/testdata/workloads/functional-query/queries/QueryTest/kudu-show-create.test File testdata/workloads/functional-query/queries/QueryTest/kudu-show-create.test: Line 9: ) > Ya the create managed table part will be done later. It's just in a weird s Ah yes, but that's not the only thing: also that this comes back with the storage_handler tbl property set, and the code rejects that. I think we may need to just allow setting either STORED AS KUDU or the storage handler property. http://gerrit.cloudera.org:8080/#/c/2617/3/tests/conftest.py File tests/conftest.py: Line 264: conn > The tests use this stuff. It seems pretty small compared to the other stuff If you don't mind pulling test infra stuff into a separate review I think it'd be helpful to keep this CR a bit more focused and also for someone like Michael to review this as well. -- 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: 3 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
