impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4379: Throw if Kudu table created with var/char
Date Thu, 27 Oct 2016 01:51:13 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4379: Throw if Kudu table created with var/char
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4857/1//COMMIT_MSG
Commit Message:

PS1, Line 10: analysis
> Actually, this is not correct. We throw an ImpalaRuntimeException in the ca
Good point. I thought about this and I think that ideally we would do this check during analysis,
but we can't easily do it for EXTERNAL tables right now since the kudu table loading happens
in the catalog. While we could do it for internal tables, I don't think it's worth having
2 different failure paths. What do you think? If we keep it this way I'll fix this comment.


http://gerrit.cloudera.org:8080/#/c/4857/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java:

PS1, Line 231: String storageHandler = properties.get(KuduTable.KEY_STORAGE_HANDLER);
             :     Preconditions.checkState(!Strings.isNullOrEmpty(storageHandler));
             :     if (!storageHandler.equals(KuduTable.KUDU_STORAGE_HANDLER))
> Maybe we can replace all this with if (!KuduTable.isKuduTable(msTbl)) which
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I475273cbbf4110db8d0f78ddf9a56abfc6221e3e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message