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-4828: Alter Kudu schema outside Impala may crash on read
Date Sat, 04 Feb 2017 00:48:52 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4828: Alter Kudu schema outside Impala may crash on read
......................................................................


Patch Set 1:

(5 comments)

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

Line 20: open, even if the schema changes concurrently.
> It would probably be worth adding that this is the earliest point and the c
Done


http://gerrit.cloudera.org:8080/#/c/5840/1/be/src/exec/kudu-scanner.cc
File be/src/exec/kudu-scanner.cc:

PS1, Line 133: for (int i = 0; i < tuple_desc->slots().size(); ++i) {
> As we spoke, if an ALTER TABLE is done and a column is removed, a crash mig
Really good point.

I handled the cases where there are more or fewer cols than expected. If there are fewer,
I fail the query. If there are more, I add a warning but then continue. I added new test cases.


Line 150:       "Unable to deserialize scan token");
> a quick comment explaining why this is needed would be helpful.
Done


http://gerrit.cloudera.org:8080/#/c/5840/1/be/src/exec/kudu-util.cc
File be/src/exec/kudu-util.cc:

PS1, Line 77: PrimitiveType KuduColTypeToPrimitiveType(
            :     const KuduColumnSchema::DataType& type) {
> This may be an over optimization, but what if we had an array mapping KuduC
Good idea but we only use this on a per-partition basis, so not really on the hot path. Lets
change it if it ever shows up in perf.


http://gerrit.cloudera.org:8080/#/c/5840/1/common/thrift/generate_error_codes.py
File common/thrift/generate_error_codes.py:

PS1, Line 314: is type
> nit: is of type
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6d43f5bb9811e728ad592933066d006c8fb4553a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message