drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jacques Nadeau" <jacques.dr...@gmail.com>
Subject Re: Review Request 27711: To fix DRILL-1062, honoring "NULLS FIRST" and "NULLS LAST"
Date Sun, 08 Feb 2015 20:59:28 GMT

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



exec/java-exec/src/main/codegen/templates/ComparisonFunctions.java
<https://reviews.apache.org/r/27711/#comment117366>

    it seems like it would be much cleaner/easier if you just set the output string you want
once before writing the code such as:
    
    <#if nullComparesHigh>
    nullComparisonLeft = "-1"
    nullComparisonRight = "1"
    <#else>
    nullComparisonLeft = "1"
    nullComparisonRight = "-1"
    </#if>
    
    And then you can just do ${output} = ${nullComparisonLeft} or similar in the code.  Would
make the code easier to read/maintain I think.
    
    Note, I didn't actually spend any time thinking about what should be -1 and 1 in the code
above, it was only to illustrate.



exec/java-exec/src/main/codegen/templates/DateIntervalFunctions.java
<https://reviews.apache.org/r/27711/#comment117368>

    I think Var comparisons should be considered MEDIUM and all others should be SIMPLE. 
This is comparison to things like date parsing, which would be complex.



exec/java-exec/src/main/codegen/templates/Decimal/CastVarCharDecimal.java
<https://reviews.apache.org/r/27711/#comment117369>

    If you want to put todo in this code, put inside a freemarker comment.  Otherwise, runtime
compilation of this code will go slower.  
    
    In general, this whole piece of code (for all of these decimal things) should be pulled
out as separate classes/static methods similar to ByteFunctionHelpers.  That should be in
the todo.


- Jacques Nadeau


On Feb. 6, 2015, 11:22 p.m., Daniel Barclay wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27711/
> -----------------------------------------------------------
> 
> (Updated Feb. 6, 2015, 11:22 p.m.)
> 
> 
> Review request for drill, Jinfeng Ni and Mehant Baid.
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> Main change:  Augmented *ordering* comparison function templates
> and calls to to order NULL values correctly (per "NULLS FIRST",
> "NULLS LAST", or correct default (depending on whether ordering is
> ascending or descending)
> *  Cloned each "compare_to" function template into 
>    "compare_to_nulls_high" and "compare_to_nulls_low" versions
>    and adjusted to handle NULL correctly.
> *  Added corresponding new version of getComparator(...).
> *  Updated code around calls to getComparator(...) re NULL ordering.
> *  Added test class and test data files.
> 
> 
> Diffs
> -----
> 
>   .gitignore 838ea6b 
>   common/src/main/java/org/apache/drill/common/logical/data/Order.java dada606 
>   exec/java-exec/src/main/codegen/data/CompareTypes.tdd f384d52 
>   exec/java-exec/src/main/codegen/templates/ComparisonFunctions.java 628277c 
>   exec/java-exec/src/main/codegen/templates/DateIntervalFunctions.java 8fe13bb 
>   exec/java-exec/src/main/codegen/templates/Decimal/CastVarCharDecimal.java 960368a 
>   exec/java-exec/src/main/codegen/templates/Decimal/DecimalFunctions.java 0c4af01 
>   exec/java-exec/src/main/java/org/apache/drill/exec/expr/annotations/FunctionTemplate.java
1f732a3 
>   exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionGenerationHelper.java
d007d7c 
>   exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/ComparisonFunctions.java
bf42ce6 
>   exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/ComparisonFunctionsNullable.java
570aaeb 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/TopNBatch.java
9829fc6 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/StreamingAggBatch.java
860627d 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java
257b93e 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java
d78ba8e 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java
a062074 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortBatch.java
19f5423 
>   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/xsort/ExternalSortBatch.java
9026661 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java
9015a16 
>   exec/jdbc/src/test/java/org/apache/drill/jdbc/test/JdbcNullOrderingAndGroupingTest.java
PRE-CREATION 
>   exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcQuery.java b627c38 
>   exec/jdbc/src/test/resources/donuts.json PRE-CREATION 
>   exec/jdbc/src/test/resources/null_ordering_and_grouping_data.json PRE-CREATION 
>   pom.xml 17f0e09 
> 
> Diff: https://reviews.apache.org/r/27711/diff/
> 
> 
> Testing
> -------
> 
> Ran new fix-specific unit tests. 
> 
> 
> Thanks,
> 
> Daniel Barclay
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message