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