----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21168/#review42578 -----------------------------------------------------------
Design looks good. Few implementation related comments. ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java <https://reviews.apache.org/r/21168/#comment76384> Instead of adding noop functions statically, we should put these functions in test/ package and than do add jar for testing. Multiple reasons: * Better to isolate test code from production code. * It will also exercise add jar functionality for PTF functions for which I am not sure we have coverage. * These functions also show up in default list of inbuilt functions. It may confuse user to wonder what good these functions are for. show_functions.q failed because of this. ql/src/java/org/apache/hadoop/hive/ql/exec/PTFOperator.java <https://reviews.apache.org/r/21168/#comment76385> Can you add a comment why we need to keep track for first row processed in Map? ql/src/java/org/apache/hadoop/hive/ql/exec/PTFOperator.java <https://reviews.apache.org/r/21168/#comment76394> Nice helpful comments! ql/src/java/org/apache/hadoop/hive/ql/exec/PTFOperator.java <https://reviews.apache.org/r/21168/#comment76386> Better name : outputPartRowsItr? ql/src/java/org/apache/hadoop/hive/ql/parse/PTFTranslator.java <https://reviews.apache.org/r/21168/#comment76391> This doesnt feel right. Why do we need to special handle no-op functions? If there is a good reason for it, we should document in comment here. ql/src/java/org/apache/hadoop/hive/ql/udf/ptf/NoopStreaming.java <https://reviews.apache.org/r/21168/#comment76392> Better placed in test package? Also, ASF headers are missing. ql/src/java/org/apache/hadoop/hive/ql/udf/ptf/NoopWithMapStreaming.java <https://reviews.apache.org/r/21168/#comment76393> Better placed in test package? Also, ASF headers are missing. ql/src/java/org/apache/hadoop/hive/ql/udf/ptf/TableFunctionEvaluator.java <https://reviews.apache.org/r/21168/#comment76395> Nice helpful comments! ql/src/java/org/apache/hadoop/hive/ql/udf/ptf/TableFunctionEvaluator.java <https://reviews.apache.org/r/21168/#comment76396> Comment made sense. Since like those fields are not present in class any more. Shall we just get rid of this? ql/src/java/org/apache/hadoop/hive/ql/udf/ptf/TableFunctionEvaluator.java <https://reviews.apache.org/r/21168/#comment76397> Better name: canAcceptInputAsStream? - Ashutosh Chauhan On May 7, 2014, 6:11 p.m., Harish Butani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21168/ > ----------------------------------------------------------- > > (Updated May 7, 2014, 6:11 p.m.) > > > Review request for hive and Ashutosh Chauhan. > > > Bugs: HIVE-6999 > https://issues.apache.org/jira/browse/HIVE-6999 > > > Repository: hive-git > > > Description > ------- > > There are a set of use cases where the Table Function can operate on a > Partition row by row or on a subset(window) of rows as it is being streamed > to it. > Windowing has couple of use cases of this:processing of Rank functions, > processing of Window Aggregations. > But this is a generic concept: any analysis that operates on an Ordered > partition maybe able to operate in Streaming mode. > This patch introduces streaming mode in PTFs and provides the mechanics to > handle PTF chains that contain both modes of PTFs. > Subsequent patches will introduce Streaming mode for Windowing. > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java 3bb8fa9 > ql/src/java/org/apache/hadoop/hive/ql/exec/PTFOperator.java 4d314b7 > ql/src/java/org/apache/hadoop/hive/ql/parse/PTFTranslator.java 34aebf0 > ql/src/java/org/apache/hadoop/hive/ql/udf/ptf/NoopStreaming.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/udf/ptf/NoopWithMapStreaming.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/udf/ptf/TableFunctionEvaluator.java > 1087bbf > ql/src/test/queries/clientpositive/ptf_streaming.q PRE-CREATION > ql/src/test/results/clientpositive/ptf_streaming.q.out PRE-CREATION > > Diff: https://reviews.apache.org/r/21168/diff/ > > > Testing > ------- > > added new tests > > > Thanks, > > Harish Butani > >
