impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5664: Unix time to timestamp conversions may crash Impala
Date Thu, 14 Sep 2017 19:18:50 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5664: Unix time to timestamp conversions may crash Impala
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7954/2/be/src/runtime/timestamp-test.cc
File be/src/runtime/timestamp-test.cc:

PS2, Line 637: FromUnixTime
The problem is only with the functions that take sub-second resolution, right? I think that's
important to clarify -- otherwise it's not clear what we mean by rounding.


PS2, Line 685: n
nit: typo


http://gerrit.cloudera.org:8080/#/c/7954/2/be/src/runtime/timestamp-value.h
File be/src/runtime/timestamp-value.h:

PS2, Line 80: !date_.is_special()
how about HasDate()?  But also, should IsValidDate() be checking HasDate()? That code looks
bogus if date_.is_special(), so the call from ScalarColumnReader<TimestampValue, true>::ValidateSlot()
seems dangerous (if the parquet file contained a corrupted value that corresponded to not_a_date_time).


PS2, Line 81: ptime
> There was no exception thrown at the time of creation, but much later, when
We don't want to use exceptions in impala. We limit their use to when interacting with other
libraries. So not throwing an exception here is the right thing.


Line 86:     }
why is that that the year() calls in timestamp-functions-ir.cc aren't sufficient, e.g.:

    // Validate that the ptime is not "special" (ie not_a_date_time) and has a valid year.
    // If validation fails, an exception is thrown.
    datetime.date().year();

Centralizing where we do this validation seems like a good idea, but unlike the other code
that attempts to do this kind of thing, e.g. TimestampFunctions::AddSub(), we don't get a
warning. The user should probably get a warning in this case.
2) If we're going to do the validation in a central place, we should try to clean up the other
code that does the year() calls (assuming it's trying to validate the same thing).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I77b2f6284d3a597f57e61c17a67c959eff9e38ff
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message