impala-reviews mailing list archives

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

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


Patch Set 2:

(2 comments)

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()?
I looked at ScalarColumnReader<TimestampValue, true>::ValidateSlot(), and now I see
that TimestampValue is created via reinterpret_cast on buffer pointers - this means that its
constructor is skipped, so it is not possible to force validity in the public functions.

>That code looks bogus if date_.is_special(), 
Special values lie outside the valid time range, so IsValidDate can only return true if is_special()
is false.

TimestampValue can have not_a_date_time in date_ or time_, it is only invalid, if both are
not_a_date_time. I am not sure, what will/should happen in these cases.


Line 86:     }
> why is that that the year() calls in timestamp-functions-ir.cc aren't suffi
About giving a warning to the user: is it ok to log in low level classes like TimestampValue?
AddSub calls FunctionContext::AddWarning, while TimestampValue`s functions do not receive
FunctionContext arguments.

If TimestampValue functions cannot log, then I will have to look for their callers and add
checks + warnings there.

Note that I am not familiar with Impala`s logging system yet.


-- 
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