----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21970/#review44178 -----------------------------------------------------------
Mostly looks good. Some minor comments. ql/src/java/org/apache/hadoop/hive/ql/exec/PTFPartition.java <https://reviews.apache.org/r/21970/#comment78495> Seems like there is no call of this constructor with createElemContainer = false. So, is this new boolean required ? ql/src/java/org/apache/hadoop/hive/ql/exec/PTFRollingPartition.java <https://reviews.apache.org/r/21970/#comment78500> For more clarity.. it will help to show these four indices using ASCII art, like following : -------------------------------------- | | | | ql/src/java/org/apache/hadoop/hive/ql/exec/PTFRollingPartition.java <https://reviews.apache.org/r/21970/#comment78497> I think we can do new ArrayList(precedingSpan + followingSpan). Also, since we currently support only fixed size windows, this could even be an array. ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFAverage.java <https://reviews.apache.org/r/21970/#comment78501> Good to add comment that range based windows not supported yet. ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFAverage.java <https://reviews.apache.org/r/21970/#comment78502> Add comment that unbounded windows not supported yet. ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFRank.java <https://reviews.apache.org/r/21970/#comment78503> Do we need this boolean? All ranking functions can support streaming.. isn't it? ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFStreamingEnhancer.java <https://reviews.apache.org/r/21970/#comment78504> Better name : GenericUDAFStreamingEvaluator ? ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFStreamingEnhancer.java <https://reviews.apache.org/r/21970/#comment78505> This can always be computed using preceding + following, so we can get rid of it. ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFStreamingEnhancer.java <https://reviews.apache.org/r/21970/#comment78510> Can you add a comment for this? especially last factor of wSize * underlying. ql/src/java/org/apache/hadoop/hive/ql/udf/ptf/WindowingTableFunction.java <https://reviews.apache.org/r/21970/#comment78511> Better name : funcArgs? - Ashutosh Chauhan On May 28, 2014, 5:40 a.m., Harish Butani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21970/ > ----------------------------------------------------------- > > (Updated May 28, 2014, 5:40 a.m.) > > > Review request for hive and Ashutosh Chauhan. > > > Bugs: HIVE-7062 > https://issues.apache.org/jira/browse/HIVE-7062 > > > Repository: hive-git > > > Description > ------- > > 1. Have the Windowing Table Function support streaming mode. > 2. Have special handling for Ranking UDAFs. > 3. Have special handling for Sum/Avg for fixed size Wdws. > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/exec/PTFOperator.java d3800c2 > ql/src/java/org/apache/hadoop/hive/ql/exec/PTFPartition.java b5adb11 > ql/src/java/org/apache/hadoop/hive/ql/exec/PTFRollingPartition.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFAverage.java > 814ae37 > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFCumeDist.java > 18c8c8d > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFDenseRank.java > c1d43d8 > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFEvaluator.java > 5668a3b > > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFPercentRank.java > aab1922 > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFRank.java > 5c8f1e0 > > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFStreamingEnhancer.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFSum.java > 8508ffb > > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/ISupportStreamingModeForWindowing.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/udf/ptf/NoopStreaming.java d50a542 > ql/src/java/org/apache/hadoop/hive/ql/udf/ptf/NoopWithMapStreaming.java > be1f9ab > ql/src/java/org/apache/hadoop/hive/ql/udf/ptf/TableFunctionEvaluator.java > 8a1e085 > ql/src/java/org/apache/hadoop/hive/ql/udf/ptf/WindowingTableFunction.java > cdb5624 > ql/src/test/org/apache/hadoop/hive/ql/udaf/TestStreamingAvg.java > PRE-CREATION > ql/src/test/org/apache/hadoop/hive/ql/udaf/TestStreamingSum.java > PRE-CREATION > ql/src/test/results/clientpositive/ptf.q.out eb4997d > ql/src/test/results/clientpositive/windowing.q.out 7e23497 > ql/src/test/results/clientpositive/windowing_windowspec.q.out 6ea068c > > Diff: https://reviews.apache.org/r/21970/diff/ > > > Testing > ------- > > run existing windowing and ptf tests > Add unit tests for StreamingSum and StreamingAvg evaluators. > > > Thanks, > > Harish Butani > >
