Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Drill >> mail # dev >> Review Request 14027: Adding Limit operator end to end


Copy link to this message
-
Re: Review Request 14027: Adding Limit operator end to end


> On Sept. 9, 2013, 7:11 p.m., Julian Hyde wrote:
> > DrillLimitRule currently looks for an EnumerableLimitRel. EnumerableLimitRel is different physical implementation of limit, whereas I think DrillLimitRule should look for a logical limit -- i.e. a SortRel that has fetch or offset set. If it finds a SortRel with a fetch or limit, create a DrillLimitRel with that fetch and limit, and on top of a SortRel without any fetch or limit. If that SortRel is trivial (i.e. no sort fields) omit it.
> >
> > EnumerableLimitRule does something very similar -- you could use that code as a starting point.
> >
> > Comments would be extremely helpful. E.g. explain meaning of first and last. What do null values mean? Is the range inclusive or exclusive?
> >
> > It would simplify things if you ensure that DrillSortRel's fetch and offset fields are always null. DrillSortRule should only fire on a SortRel that has null fetch and offset. Then you can remove the call to DrillLimitRel.implementLimit from DrillSortRel. (In future you could make the drill's sort operator more efficient by applying a row limit during the sort. But let's code for the operator as it is today.)

Hi Julian, I realized I actually don't need DrillLimitRule as a LIMIT in optiq is translated to a SortRel afterwards.

As for first and last, I'm following the Logical Operator doc we had for drill, first and last are inclusive offsets into the result set, instead of a starting offset and fetch count like OFFSET/FETCH operators does.

I'll be adding the comments and do the changes you mentioned.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14027/#review25997
-----------------------------------------------------------
On Sept. 8, 2013, 5:37 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14027/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2013, 5:37 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Adding Limit operator end to end
>
>
> Diffs
> -----
>
>   common/src/main/java/org/apache/drill/common/logical/data/Limit.java 1774790
>   exec/java-exec/src/main/java/org/apache/drill/exec/opt/BasicOptimizer.java c116b59
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractPhysicalVisitor.java c997db4
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalVisitor.java 97e6795
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/Limit.java PRE-CREATION
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java 9984454
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/limit/LimitBatchCreator.java PRE-CREATION
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/limit/LimitRecordBatch.java PRE-CREATION
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/StatsCollector.java 2ef5295
>   exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/limit/TestSimpleLimit.java PRE-CREATION
>   exec/java-exec/src/test/resources/limit/test1.json PRE-CREATION
>   sqlparser/pom.xml 84b17a0
>   sqlparser/src/main/java/org/apache/drill/optiq/DrillLimitRel.java PRE-CREATION
>   sqlparser/src/main/java/org/apache/drill/optiq/DrillLimitRule.java PRE-CREATION
>   sqlparser/src/main/java/org/apache/drill/optiq/DrillOptiq.java e687435
>   sqlparser/src/main/java/org/apache/drill/optiq/DrillSortRel.java 64995c5
>   sqlparser/src/main/java/org/apache/drill/optiq/DrillSortRule.java 0d9852a
>
> Diff: https://reviews.apache.org/r/14027/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>