> On Sept. 18, 2013, 6:39 p.m., Jacques Nadeau wrote: > > sqlparser/src/main/java/org/apache/drill/optiq/DrillLimitRule.java, line 38 > > <https://reviews.apache.org/r/14027/diff/2/?file=350257#file350257line38> > > > > It looks like you are currently inserting a limit rel for all sorts > > now. I think we should only do that iff the sort has limit settings.
I'm actually only calling the LimitRule if either offset or fetch is defined, which is the same as what you described. It looks odd most likely because Julian's impl in Optiq converts all LIMIT/OFFSET calls even without a ORDER BY into a SortRel without any field to order with. > On Sept. 18, 2013, 6:39 p.m., Jacques Nadeau wrote: > > sqlparser/src/main/java/org/apache/drill/optiq/DrillLimitRel.java, line 30 > > <https://reviews.apache.org/r/14027/diff/2/?file=350256#file350256line30> > > > > We're trying to move towards builder paradigm with these rather than > > hand creating the json. E.g. just make a Limit object and then add a > > implementor.add(LogicalOperator op) which does the conversion. Julian's > > original code doesn't follow this convention but we're trying to get there > > so I would be ideal if you could set the groundwork for this. Let me know if you like the setup I did. - Timothy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14027/#review26231 ----------------------------------------------------------- On Sept. 10, 2013, 8:29 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14027/ > ----------------------------------------------------------- > > (Updated Sept. 10, 2013, 8:29 a.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 > exec/ref/src/main/java/org/apache/drill/exec/ref/rops/LimitROP.java 4a29f94 > 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 > sqlparser/src/test/java/org/apache/drill/jdbc/test/FullEngineTest.java > a1a6cf2 > sqlparser/src/test/java/org/apache/drill/jdbc/test/JdbcTest.java 37e81b7 > > Diff: https://reviews.apache.org/r/14027/diff/ > > > Testing > ------- > > > Thanks, > > Timothy Chen > >
