Re: Review Request 30051: DRILL-1908: new window function implementation

[abdelhakim deneche <ad...@gmail.com>]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30051/
-----------------------------------------------------------

(Updated March 3, 2015, 10 p.m.)


Review request for drill and Jacques Nadeau.


Changes
-------

patch rebased to master


Bugs: DRILL-1908
    https://issues.apache.org/jira/browse/DRILL-1908


Repository: drill-git


Description
-------

In order to fix DRILL-1487 a complete rewrite of the StreamingWindowFrameRecordBatch was needed. This patch adds a new WindowFrameRecordBatch that correctly handles window functions with or without order by clauses.
This code still lacks support for frame clauses and may be optimized to reduce unneeded frame computations.


Diffs (updated)
-----

  common/src/main/java/org/apache/drill/common/logical/data/AbstractBuilder.java 28424a5 
  common/src/main/java/org/apache/drill/common/logical/data/Window.java 6dba77c 
  contrib/data/pom.xml 86075f2 
  contrib/data/window-test-data/pom.xml PRE-CREATION 
  exec/java-exec/pom.xml 28eca2b 
  exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java 5efcce8 
  exec/java-exec/src/main/java/org/apache/drill/exec/opt/BasicOptimizer.java 5288f5d 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/WindowPOP.java 17738ee 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/OverFinder.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/StreamingWindowFrameBatchCreator.java 9b8929f 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/StreamingWindowFrameRecordBatch.java 87209eb 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/StreamingWindowFrameTemplate.java e2c7e9e 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/StreamingWindowFramer.java 9588cef 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameBatchCreator.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameRecordBatch.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameTemplate.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFramer.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRuleSets.java 3b7adca 
  exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/StreamingWindowPrel.java f1a8bc0 
  exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/StreamingWindowPrule.java 00c20b2 
  exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrule.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java 232778a 
  exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 3d3e96f 
  exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/window/TestWindowFrame.java a9d2ef8 
  exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestWindowFunctions.java 6eff6db 

Diff: https://reviews.apache.org/r/30051/diff/


Testing
-------

Unit tests are available to test window functions in mulitple scenarios:
- b1.p1: single batch with a single partition
- b1.p2: 2 batches, each containing a different parition
- b2.p4: 2 batches and 4 partitions, one partition has rows in both batches
- b3.p2: 3 batches and 2 partitions, one partition includes the whole 2nd batch and has rows in 3 batches
- b4.p4: 4 batches and 4 partitions, the partitions are arranged to test an edge case: the 2nd time innerNext() is called, WindowFrameRecordBatch has enough saved batches to call it's framer.doWork() without the need to call next(incoming)

All tests, except the last one, come in 2 variations: with and without "order by" clause

all unit tests pass. functional, sf100 and customer tests don't add any new failures


Thanks,

abdelhakim deneche