Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201702032004.v13K4EjK032049@ip-10-146-233-104.ec2.internal>
Date: Fri, 3 Feb 2017 20:04:14 +0000
From: "Sailesh Mukil (Code Review)" <gerrit@cloudera.org>
To: Matthew Jacobs <mj@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Dan Hecht <dhecht@cloudera.com>,
        Thomas Tauber-Marshall <tmarshall@cloudera.com>
Reply-To: sailesh@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4828=3A_Alter_Kudu_schema_outside_Impala_may_crash_on_read=0A?=
In-Reply-To: <gerrit.1485903880000.I6d43f5bb9811e728ad592933066d006c8fb4553a@gerrit.cloudera.org>
References: <gerrit.1485903880000.I6d43f5bb9811e728ad592933066d006c8fb4553a@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Fri, 03 Feb 2017 20:04:25 -0000

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