> 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 > >
