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 <m...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
