----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27711/#review71537 -----------------------------------------------------------
Went through the first couple of files. Will do the rest later. common/src/main/java/org/apache/drill/common/logical/data/Order.java <https://reviews.apache.org/r/27711/#comment117281> Agree. The draft SQL standard 2003 says if null ordering is not specified, then an impleleation-defined null ordering is used. Both Oracle and Postgresql seem to use the same policy: NULL_LAST for ASC, and NULL_FIRST for DESC. I think Drill should adopt the same policy here. Please addd codes to set the Default null ordering based on ASC/DESC. common/src/main/java/org/apache/drill/common/logical/data/Order.java <https://reviews.apache.org/r/27711/#comment117282> Agree. The type of STRICTLY_DESCENDING and CLUSTERED seem to be defined in optiq library only. SQL standard does not have those concepts. I think we should only accept DESCENDING / ASCENDING here, and throw UnsupportedOperationException for all other cases. exec/java-exec/src/main/codegen/templates/ComparisonFunctions.java <https://reviews.apache.org/r/27711/#comment117288> Is it true that "compare_to_nulls_high" or "compare_to_nulls_low" only needed when either side is nullable? That is, we should not have these two functions in GCompareBigIntBigInt class ? exec/java-exec/src/main/codegen/templates/DateIntervalFunctions.java <https://reviews.apache.org/r/27711/#comment117289> Why do we keep another template for DateInterval types? Is it possible that it's erged with ComparisonFunctions.java? I saw some logic are identical. exec/java-exec/src/main/codegen/templates/Decimal/CastVarCharDecimal.java <https://reviews.apache.org/r/27711/#comment117290> Can you move those TODO into a different JIRA, if you believe that's the desired result? Seems that is not related to NULL FIRST/NULL LAST issue. - Jinfeng Ni On Feb. 6, 2015, 3: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, 3: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 > >
