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