Review Board 1.7.22


Review for PIG-1314 - add datetime type in pig

Review Request #5414 - Created June 19, 2012 and updated

Thejas Nair
PIG-1314
Reviewers
pig
pig
Review for PIG-1314

 
Total:
25
Open:
24
Resolved:
1
Dropped:
0
Status:
From:
Description From Last Updated Status
The Long.valueOf is unnecessary. If the value can't be stored in an integer, it should result in a warning. See ... Thejas Nair July 12, 2012, 1:59 a.m. Open
It will be more readable to use {} for the if statement to make it more readable. Thejas Nair July 12, 2012, 1:59 a.m. Open
I think it makes sense to support conversion to all numeric types. That will be consistent with other conversions. Thejas Nair July 12, 2012, 1:59 a.m. Open
I think it is ok to allow conversions from double and float as well, we allow the conversions to long ... Thejas Nair July 12, 2012, 1:59 a.m. Open
outputSchema() implementation is unnecessary because the output type is simple and does not depend on input schema. Most or all ... Thejas Nair July 12, 2012, 1:59 a.m. Open
We need to propagate the timezone from the client to backend - where the udfs get executed. Thejas Nair July 12, 2012, 1:59 a.m. Open
indentation cleanup can be done here Thejas Nair July 12, 2012, 1:59 a.m. Open
this comment seems unrelated to this udf Thejas Nair July 12, 2012, 1:59 a.m. Open
javadoc is not correct for this udf Thejas Nair July 12, 2012, 1:59 a.m. Open
i am wondering if we should use Duration.toStandardDays() . But I think this approach might be more performant as it ... Thejas Nair July 12, 2012, 1:59 a.m. Open
The example will be more readable with just one date column. Ie ISOin = LOAD 'test.tsv' USING PigStorage('\t') AS (dt:datetime); Thejas Nair July 12, 2012, 1:59 a.m. Open
It says AddDuration here Thejas Nair July 12, 2012, 1:59 a.m. Open
need to document the different input arguments. Thejas Nair July 12, 2012, 1:59 a.m. Open
we should avoid using exceptions as part of normal code path. It is better to separate this into two udfs ... Thejas Nair July 12, 2012, 1:59 a.m. Open
does this argTOFuncMapping use work ? I think you would need to add a different schema for each possible combination ... Thejas Nair July 12, 2012, 1:59 a.m. Open
we need to use the default timezone if it is not specified. (this has to be shipped to backend). Thejas Nair July 12, 2012, 1:59 a.m. Open
unix time is seconds, not milliseconds Thejas Nair July 12, 2012, 1:59 a.m. Open
we should refactor this timezone extraction code so that it can be re-used in the toDate udf . Thejas Nair July 12, 2012, 1:59 a.m. Open
can we just compare the longs ? That way we can avoid the object creation. creating objects reduces the performance ... Thejas Nair July 12, 2012, 1:59 a.m. Open
as we allow long to be cast to float/double, i think it will be more consistent to allow that for ... Thejas Nair July 12, 2012, 1:59 a.m. Open
we need to deal with timezone in the date string Thejas Nair July 12, 2012, 1:59 a.m. Open
how did you arrive at this number ? Thejas Nair July 12, 2012, 1:59 a.m. Open
check for DATETIME should be not added here. Thejas Nair July 12, 2012, 1:59 a.m. Open
check for DATETIME should be not added here. Thejas Nair July 12, 2012, 1:59 a.m. Open
Review request changed
Updated (Aug. 17, 2012, 12:09 a.m.)
PIG-1314-6.patch