impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4363: Add Parquet timestamp validation
Date Mon, 28 Nov 2016 18:35:01 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4363: Add Parquet timestamp validation

Patch Set 8:

File be/src/exec/

Line 473:     ErrorMsg* msg = NULL;
see comment below. this pointer is never freed.  what's the objection to putting the logic
inside ValidateSlot() so that the memory allocation can stay automatic?

Line 484:       // This point is reached if slot validation succeeds, but slot conversion
but don't we need to validate the converted value? i.e. the timestamp might be within bounds
before conversion, but not after. do we handle that?

Line 603:     *msg = new ErrorMsg(TErrorCode::PARQUET_TIMESTAMP_OUT_OF_RANGE,
this memory is leaked.
File be/src/runtime/timestamp-value.h:

PS7, Line 160: g buffer. The size of the buffer should be a
> The result of a select statement would look like, for example:
What I'm asking is: don't some days legitimately have more than NUMBER_OF_NANOSECNODS_IN_24H
when leap seconds are in play?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I9988449aa0dc0f39fabb91ce6cce0dd8a06e8bcf
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Laszlo Gaal <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message