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
Jacques Nadeau 2013-09-18, 18:39

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

exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/limit/LimitRecordBatch.java
<https://reviews.apache.org/r/14027/#comment51255>

    Two questions/concerns here:
    
    It is possible that the limit might somewhere other than the first record batch isn't long enough to contain the records needed but a subsequent batch contains the requested records.  For example, if the requested limit is 100..200 but each batch only carries 50 records, you will return zero records as far as I can see.
    
    Secondly, since you're not necessarily consuming the entire upstream record batch, you should tell it to stop using kill so that those don't just hang/remain and stop reading upstream data.

sqlparser/src/main/java/org/apache/drill/optiq/DrillLimitRel.java
<https://reviews.apache.org/r/14027/#comment51256>

    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.

sqlparser/src/main/java/org/apache/drill/optiq/DrillLimitRule.java
<https://reviews.apache.org/r/14027/#comment51253>

    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.

sqlparser/src/main/java/org/apache/drill/optiq/DrillSortRule.java
<https://reviews.apache.org/r/14027/#comment51258>

    This seems weird/wrong.  Maybe it is just the comment.  Should the comment read: // this rule only supports conversion of sorts that don't have offset and fetch.
    
    I guess the part that is weird is rules should be independent.  And there is no guarantee of what order they are applied in so referring to another rule in a rule seems wrong.  Am I missing something here?

sqlparser/src/test/java/org/apache/drill/jdbc/test/JdbcTest.java
<https://reviews.apache.org/r/14027/#comment51260>

    Can you please modify this so that do a json comparison rather than a string comparison?  These comparisons are so brittle as a change in json spacing or key ordering will fail them even though the output is still valid.
- Jacques Nadeau
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