Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8936 )

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
......................................................................


Patch Set 18:

(2 comments)

Looks good but we'll want to merge after the IMPALA-6995 fix so we don't hit 
that DCHECK in the meantime.

http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/runtime/timestamp-parse-util.cc
File be/src/runtime/timestamp-parse-util.cc:

http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/runtime/timestamp-parse-util.cc@370
PS15, Line 370:   DCHECK_GT(len, 0);
> Introduced a DCHECK_GT(len, 0) and a check in the caller.
The checks added in this patch don't really fix the problem (and actually make 
it worse) because it has other callsites. Filed IMPALA-6995 to fix this 
properly.


http://gerrit.cloudera.org:8080/#/c/8936/18/be/src/runtime/timestamp-parse-util.cc
File be/src/runtime/timestamp-parse-util.cc:

http://gerrit.cloudera.org:8080/#/c/8936/18/be/src/runtime/timestamp-parse-util.cc@370
PS18, Line 370:   DCHECK_GT(len, 0);
Please remove this, it's not valid. E.g. cast('' as timestamp).

We can wait for the fix for IMPALA-6995 to go in before merging this change.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 18
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Reviewer: anujphadke <apha...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 May 2018 22:05:28 +0000
Gerrit-HasComments: Yes

Reply via email to