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

[Chris Westin <cw...@yahoo.com>]

> On Jan. 30, 2015, 1:12 a.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/GenerateTestData.java, line 121
> > <https://reviews.apache.org/r/30051/diff/2/?file=826885#file826885line121>
> >
> >     Make the file prefix a command-line argument so that people besides yourself can run this.
> 
> abdelhakim deneche wrote:
>     Argh, I forgot to remove GenerateTestData again!
>     I just used this to generate the data used in the tests, it was never intended to be part of the final code.
>     Sorry about that

I would check it in. We might need it again in the future. What if something changes and we have to re-generate the test data?


> On Jan. 30, 2015, 1:12 a.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameTemplate.java, line 32
> > <https://reviews.apache.org/r/30051/diff/2/?file=826892#file826892line32>
> >
> >     logger should be private.
> 
> Jacques Nadeau wrote:
>     I disagree.  Our current standard is package private.  If you think we should change this throughout the code, we should have a discussion but my preference is to maintain the current standard until we decide upon a new one.

Loggers identify their source in log messages thanks to the class argument given to getLogger(). They're meant to be associated with a class in a one-to-one manner -- why else would getLogger() have this parameter?

There are no uses of the pattern "otherclass.logger...", so there's no reason for them to be package private. However, I have come across a few uses where a derived class uses the logger from its base class, and this is confusing. This has sent me looking in the wrong place for the source of the message, so we shouldn't do it. I've assumed it was accidental, and slipped by because the base class's logger wasn't private, so the author was able to use it without realizing it. Making them private will prevent that, and ensure that log messages correctly identify their real source.

Because we have not written standard that described this, and because it goes against common best practice elsewhere, I've been converting these to private wherever I've come across them. In only a couple of cases has this made me add new loggers where a derived class was accidentally using its base class's logger.


- Chris


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


On Jan. 28, 2015, 7:50 p.m., abdelhakim deneche wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30051/
> -----------------------------------------------------------
> 
> (Updated Jan. 28, 2015, 7:50 p.m.)
> 
> 
> Review request for drill.
> 
> 
> 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
> -----
> 
>   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 90734a5 
>   exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java 190c13f 
>   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 26d23f2 
>   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 79603eb 
>   exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java f20627d 
>   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
> 
> 
> Thanks,
> 
> abdelhakim deneche
> 
>