----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5414/#review9044 -----------------------------------------------------------
http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/DateTimeWritable.java <https://reviews.apache.org/r/5414/#comment19198> Please add @Override annotation for functions that are being overridden http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/POCast.java <https://reviews.apache.org/r/5414/#comment19199> The Long.valueOf is unnecessary. If the value can't be stored in an integer, it should result in a warning. See conversion from chararray to int. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/POCast.java <https://reviews.apache.org/r/5414/#comment19200> It will be more readable to use {} for the if statement to make it more readable. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/POCast.java <https://reviews.apache.org/r/5414/#comment19261> I think it makes sense to support conversion to all numeric types. That will be consistent with other conversions. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/POCast.java <https://reviews.apache.org/r/5414/#comment19262> I think it is ok to allow conversions from double and float as well, we allow the conversions to long or int. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/AddDuration.java <https://reviews.apache.org/r/5414/#comment19263> outputSchema() implementation is unnecessary because the output type is simple and does not depend on input schema. Most or all datetime udfs won't need this. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/CurrentTime.java <https://reviews.apache.org/r/5414/#comment19265> We need to propagate the timezone from the client to backend - where the udfs get executed. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/DaysBetween.java <https://reviews.apache.org/r/5414/#comment19266> indentation cleanup can be done here http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/DaysBetween.java <https://reviews.apache.org/r/5414/#comment19267> this comment seems unrelated to this udf http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/DiffDate.java <https://reviews.apache.org/r/5414/#comment19268> javadoc is not correct for this udf http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/DiffDate.java <https://reviews.apache.org/r/5414/#comment19269> i am wondering if we should use Duration.toStandardDays() . But I think this approach might be more performant as it does not create an additional Duration object. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/GetDay.java <https://reviews.apache.org/r/5414/#comment19270> The example will be more readable with just one date column. Ie ISOin = LOAD 'test.tsv' USING PigStorage('\t') AS (dt:datetime); http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/SubtractDuration.java <https://reviews.apache.org/r/5414/#comment19271> It says AddDuration here http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/ToDate.java <https://reviews.apache.org/r/5414/#comment19273> need to document the different input arguments. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/ToDate.java <https://reviews.apache.org/r/5414/#comment19274> we should avoid using exceptions as part of normal code path. It is better to separate this into two udfs classes, and let pig pick the right one based on input types (the purpose of getArgsToFuncMapping). http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/ToDate.java <https://reviews.apache.org/r/5414/#comment19272> does this argTOFuncMapping use work ? I think you would need to add a different schema for each possible combination of input tuple type. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/ToDate.java <https://reviews.apache.org/r/5414/#comment19275> we need to use the default timezone if it is not specified. (this has to be shipped to backend). http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/ToUnixTime.java <https://reviews.apache.org/r/5414/#comment19276> unix time is seconds, not milliseconds http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/Utf8StorageConverter.java <https://reviews.apache.org/r/5414/#comment19277> we should refactor this timezone extraction code so that it can be re-used in the toDate udf . http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/data/BinInterSedes.java <https://reviews.apache.org/r/5414/#comment19278> can we just compare the longs ? That way we can avoid the object creation. creating objects reduces the performance advantage of using rawcomparator . http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/data/DataType.java <https://reviews.apache.org/r/5414/#comment19279> as we allow long to be cast to float/double, i think it will be more consistent to allow that for datetime as well. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/data/DataType.java <https://reviews.apache.org/r/5414/#comment19280> we need to deal with timezone in the date string http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/data/SizeUtil.java <https://reviews.apache.org/r/5414/#comment19281> how did you arrive at this number ? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/pen/AugmentBaseDataVisitor.java <https://reviews.apache.org/r/5414/#comment19282> check for DATETIME should be not added here. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/pen/AugmentBaseDataVisitor.java <https://reviews.apache.org/r/5414/#comment19283> check for DATETIME should be not added here. - Thejas Nair On July 10, 2012, 5:41 p.m., Thejas Nair wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5414/ > ----------------------------------------------------------- > > (Updated July 10, 2012, 5:41 p.m.) > > > Review request for pig. > > > Description > ------- > > Review for PIG-1314 > > > This addresses bug PIG-1314. > https://issues.apache.org/jira/browse/PIG-1314 > > > Diffs > ----- > > http://svn.apache.org/repos/asf/pig/trunk/conf/pig.properties 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/DBStorage.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/HiveColumnarLoader.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/SequenceFileLoader.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/LoadCaster.java > 1359212 > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/PigServer.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/PigWarning.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/StoreCaster.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/DateTimeWritable.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/HDataType.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigDateTimeRawComparator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/PhysicalOperator.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/ConstantExpression.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/EqualToExpr.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/ExpressionOperator.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/GTOrEqualToExpr.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/GreaterThanExpr.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/LTOrEqualToExpr.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/LessThanExpr.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/NotEqualToExpr.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/POBinCond.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/POCast.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/POIsNull.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/POUserFunc.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POForEach.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POLocalRearrange.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPreCombinerLocalRearrange.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POSort.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/hbase/HBaseBinaryConverter.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/hbase/HBaseStorage.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/AddDuration.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/BinStorage.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/CurrentTime.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/DaysBetween.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/DiffDate.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/GetDay.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/GetHour.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/GetMinute.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/GetMonth.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/GetSecond.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/GetYear.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/HoursBetween.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/MinutesBetween.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/MonthsBetween.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/SecondsBetween.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/SubtractDuration.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/TextLoader.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/ToDate.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/ToString.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/ToUnixTime.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/Utf8StorageConverter.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/YearsBetween.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/data/BinInterSedes.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/data/DataReaderWriter.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/data/DataType.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/data/DefaultTuple.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/data/SchemaTupleClassGenerator.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/data/SizeUtil.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/data/TypeAwareTuple.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/impl/io/NullableDateTimeWritable.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/impl/logicalLayer/schema/SchemaUtil.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/impl/util/CastUtils.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/impl/util/NumValCarrier.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/impl/util/StorageUtil.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/visitor/TypeCheckingExpVisitor.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AliasMasker.g > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstPrinter.g > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstValidator.g > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanGenerator.g > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/QueryLexer.g > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/QueryParser.g > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/pen/AugmentBaseDataVisitor.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/scripting/jruby/JrubyScriptEngine.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/scripting/jruby/RubySchema.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/udfs/java/org/apache/pig/test/udf/storefunc/PigPerformanceLoader.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestConversions.java > 1359212 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestPOCast.java > 1359212 > > Diff: https://reviews.apache.org/r/5414/diff/ > > > Testing > ------- > > > Thanks, > > Thejas Nair > >
