impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4828: Alter Kudu schema outside Impala may crash on read
Date Fri, 03 Feb 2017 20:04:14 GMT
Sailesh Mukil has posted comments on this change.

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


Patch Set 1:

(4 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 coarsest granularity
at which we can successfully detect a schema change if there is any, without leaving room
for error, just to assuage any concerns of whether we're doing this check too often.


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 might occur in this
loop.

Another point I wanted to call out: What's the expected behavior when a column is added?
 1) Currently, this might pass and we will probably return the results for N-1 columns successfully.
Is that what we want?

 2) Or should we fail the query?

 3) Or should we return the results and also give a warning asking the user to REFRESH?


My vote is for the 3rd option, but I'm open to others take on this.


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 KuduColumnSchemas to
PrimitiveTypes? Since they both are just enums.

Just worried about the case where there are a large number of columns and we bounce on the
switch statement for every one of them for every scan token. If you don't think it's too pressing,
we can forego that.


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


-- 
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: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message