Merge pull request #1098 from katzyn/nulls

Fix some issues with NULLS FIRST / LAST

h2/src/main/org/h2/command/Parser.java

@@ -882,24 +882,30 @@ public class Parser {
         do {
             IndexColumn column = new IndexColumn();
             column.columnName = readColumnIdentifier();
+            column.sortType = parseSortType();
             columns.add(column);
-            if (readIf("ASC")) {
-                // ignore
-            } else if (readIf("DESC")) {
-                column.sortType = SortOrder.DESCENDING;
-            }
-            if (readIf("NULLS")) {
-                if (readIf("FIRST")) {
-                    column.sortType |= SortOrder.NULLS_FIRST;
-                } else {
-                    read("LAST");
-                    column.sortType |= SortOrder.NULLS_LAST;
-                }
-            }
         } while (readIfMore(true));
         return columns.toArray(new IndexColumn[0]);
     }
 
+    private int parseSortType() {
+        int sortType = 0;
+        if (readIf("ASC")) {
+            // ignore
+        } else if (readIf("DESC")) {
+            sortType = SortOrder.DESCENDING;
+        }
+        if (readIf("NULLS")) {
+            if (readIf("FIRST")) {
+                sortType |= SortOrder.NULLS_FIRST;
+            } else {
+                read("LAST");
+                sortType |= SortOrder.NULLS_LAST;
+            }
+        }
+        return sortType;
+    }
+
     private String[] parseColumnList() {
         ArrayList<String> columns = Utils.newSmallArrayList();
         do {
@@ -2029,19 +2035,7 @@ public class Parser {
                 } else {
                     order.expression = expr;
                 }
-                if (readIf("DESC")) {
-                    order.descending = true;
-                } else {
-                    readIf("ASC");
-                }
-                if (readIf("NULLS")) {
-                    if (readIf("FIRST")) {
-                        order.nullsFirst = true;
-                    } else {
-                        read("LAST");
-                        order.nullsLast = true;
-                    }
-                }
+                order.sortType = parseSortType();
                 orderList.add(order);
             } while (readIf(","));
             command.setOrder(orderList);
@@ -2701,11 +2695,7 @@ public class Parser {
         do {
             SelectOrderBy order = new SelectOrderBy();
             order.expression = readExpression();
-            if (readIf("DESC")) {
-                order.descending = true;
-            } else {
-                readIf("ASC");
-            }
+            order.sortType = parseSortType();
             orderList.add(order);
         } while (readIf(","));
         return orderList;

h2/src/main/org/h2/command/dml/Query.java

@@ -524,15 +524,14 @@ public abstract class Query extends Prepared {
                 }
             }
             index[i] = idx;
-            boolean desc = o.descending;
+            int type = o.sortType;
             if (reverse) {
-                desc = !desc;
-            }
-            int type = desc ? SortOrder.DESCENDING : SortOrder.ASCENDING;
-            if (o.nullsFirst) {
-                type += SortOrder.NULLS_FIRST;
-            } else if (o.nullsLast) {
-                type += SortOrder.NULLS_LAST;
+                // TODO NULLS FIRST / LAST should be inverted too?
+                if ((type & SortOrder.DESCENDING) != 0) {
+                    type &= ~SortOrder.DESCENDING;
+                } else {
+                    type |= SortOrder.DESCENDING;
+                }
             }
             sortType[i] = type;
         }

h2/src/main/org/h2/command/dml/SelectOrderBy.java

@@ -6,6 +6,7 @@
 package org.h2.command.dml;
 
 import org.h2.expression.Expression;
+import org.h2.result.SortOrder;
 
 /**
  * Describes one element of the ORDER BY clause of a query.
@@ -25,19 +26,9 @@ public class SelectOrderBy {
     public Expression columnIndexExpr;
 
     /**
-     * If the column should be sorted descending.
+     * Sort type for this column.
      */
-    public boolean descending;
-
-    /**
-     * If NULL should be appear first.
-     */
-    public boolean nullsFirst;
-
-    /**
-     * If NULL should be appear at the end.
-     */
-    public boolean nullsLast;
+    public int sortType;
 
     public String getSQL() {
         StringBuilder buff = new StringBuilder();
@@ -46,14 +37,7 @@ public class SelectOrderBy {
         } else {
             buff.append(columnIndexExpr.getSQL());
         }
-        if (descending) {
-            buff.append(" DESC");
-        }
-        if (nullsFirst) {
-            buff.append(" NULLS FIRST");
-        } else if (nullsLast) {
-            buff.append(" NULLS LAST");
-        }
+        SortOrder.typeToString(buff, sortType);
         return buff.toString();
     }
 

h2/src/main/org/h2/expression/Aggregate.java

@@ -252,8 +252,7 @@ public class Aggregate extends Expression {
         for (int i = 0; i < size; i++) {
             SelectOrderBy o = orderByList.get(i);
             index[i] = i + 1;
-            int order = o.descending ? SortOrder.DESCENDING : SortOrder.ASCENDING;
-            sortType[i] = order;
+            sortType[i] = o.sortType;
         }
         return new SortOrder(session.getDatabase(), index, sortType, null);
     }
@@ -590,9 +589,7 @@ public class Aggregate extends Expression {
             for (SelectOrderBy o : orderByList) {
                 buff.appendExceptFirst(", ");
                 buff.append(o.expression.getSQL());
-                if (o.descending) {
-                    buff.append(" DESC");
-                }
+                SortOrder.typeToString(buff.builder(), o.sortType);
             }
         }
         if (groupConcatSeparator != null) {
@@ -616,9 +613,7 @@ public class Aggregate extends Expression {
             for (SelectOrderBy o : orderByList) {
                 buff.appendExceptFirst(", ");
                 buff.append(o.expression.getSQL());
-                if (o.descending) {
-                    buff.append(" DESC");
-                }
+                SortOrder.typeToString(buff.builder(), o.sortType);
             }
         }
         buff.append(')');

h2/src/main/org/h2/expression/AggregateDataMedian.java

@@ -12,6 +12,7 @@ import java.util.Comparator;
 
 import org.h2.engine.Database;
 import org.h2.engine.Session;
+import org.h2.engine.SysProperties;
 import org.h2.index.Cursor;
 import org.h2.index.Index;
 import org.h2.result.SearchRow;
@@ -42,7 +43,8 @@ class AggregateDataMedian extends AggregateDataCollecting {
         IndexColumn ic = index.getIndexColumns()[0];
         int sortType = ic.sortType;
         return (sortType & SortOrder.NULLS_LAST) != 0
-                || (sortType & SortOrder.DESCENDING) != 0 && (sortType & SortOrder.NULLS_FIRST) == 0;
+                || (sortType & SortOrder.NULLS_FIRST) == 0
+                        && ((sortType & SortOrder.DESCENDING) != 0 ^ SysProperties.SORT_NULLS_HIGH);
     }
 
     /**

h2/src/main/org/h2/index/BaseIndex.java

@@ -364,6 +364,11 @@ public abstract class BaseIndex extends SchemaObjectBase implements Index {
         if (a == b) {
             return 0;
         }
+        boolean aNull = a == ValueNull.INSTANCE;
+        boolean bNull = b == ValueNull.INSTANCE;
+        if (aNull || bNull) {
+            return SortOrder.compareNull(aNull, sortType);
+        }
         int comp = table.compareTypeSafe(a, b);
         if ((sortType & SortOrder.DESCENDING) != 0) {
             comp = -comp;

h2/src/main/org/h2/result/SortOrder.java

@@ -50,20 +50,31 @@ public class SortOrder implements Comparator<Value[]> {
     public static final int NULLS_LAST = 4;
 
     /**
-     * The default sort order for NULL.
+     * The default comparison result for NULL, either 1 or -1.
      */
-    private static final int DEFAULT_NULL_SORT =
-            SysProperties.SORT_NULLS_HIGH ? 1 : -1;
+    private static final int DEFAULT_NULL_SORT;
 
     /**
-     * The default sort order bit for NULLs last.
+     * The default NULLs sort order bit for ASC indexes.
      */
-    private static final int DEFAULT_NULLS_LAST = SysProperties.SORT_NULLS_HIGH ? NULLS_LAST : NULLS_FIRST;
+    private static final int DEFAULT_ASC_NULLS;
 
     /**
-     * The default sort order bit for NULLs first.
+     * The default NULLs sort order bit for DESC indexes.
      */
-    private static final int DEFAULT_NULLS_FIRST = SysProperties.SORT_NULLS_HIGH ? NULLS_FIRST : NULLS_LAST;
+    private static final int DEFAULT_DESC_NULLS;
+
+    static {
+        if (SysProperties.SORT_NULLS_HIGH) {
+            DEFAULT_NULL_SORT = 1;
+            DEFAULT_ASC_NULLS = NULLS_LAST;
+            DEFAULT_DESC_NULLS = NULLS_FIRST;
+        } else { // default
+            DEFAULT_NULL_SORT = -1;
+            DEFAULT_ASC_NULLS = NULLS_FIRST;
+            DEFAULT_DESC_NULLS = NULLS_LAST;
+        }
+    }
 
     private final Database database;
 
@@ -116,19 +127,27 @@ public class SortOrder implements Comparator<Value[]> {
             } else {
                 buff.append('=').append(StringUtils.unEnclose(list[idx].getSQL()));
             }
-            int type = sortTypes[i++];
-            if ((type & DESCENDING) != 0) {
-                buff.append(" DESC");
-            }
-            if ((type & NULLS_FIRST) != 0) {
-                buff.append(" NULLS FIRST");
-            } else if ((type & NULLS_LAST) != 0) {
-                buff.append(" NULLS LAST");
-            }
+            typeToString(buff.builder(), sortTypes[i++]);
         }
         return buff.toString();
     }
 
+    /**
+     * Appends type information (DESC, NULLS FIRST, NULLS LAST) to the specified statement builder.
+     * @param builder statement builder
+     * @param type sort type
+     */
+    public static void typeToString(StringBuilder builder, int type) {
+        if ((type & DESCENDING) != 0) {
+            builder.append(" DESC");
+        }
+        if ((type & NULLS_FIRST) != 0) {
+            builder.append(" NULLS FIRST");
+        } else if ((type & NULLS_LAST) != 0) {
+            builder.append(" NULLS LAST");
+        }
+    }
+
     /**
      * Compare two expressions where one of them is NULL.
      *
@@ -292,9 +311,9 @@ public class SortOrder implements Comparator<Value[]> {
      * @param sortType sort type bit mask
      * @return bit mask with either {@link #NULLS_FIRST} or {@link #NULLS_LAST} explicitly set.
      */
-    public static int addExplicitNullPosition(final int sortType) {
-        if ((sortType & NULLS_FIRST) != NULLS_FIRST && (sortType & NULLS_LAST) != NULLS_LAST) {
-            return sortType | ((sortType & DESCENDING) == ASCENDING ? DEFAULT_NULLS_LAST : DEFAULT_NULLS_FIRST);
+    public static int addExplicitNullPosition(int sortType) {
+        if ((sortType & (NULLS_FIRST | NULLS_LAST)) == 0) {
+            return sortType | ((sortType & DESCENDING) == 0 ? DEFAULT_ASC_NULLS : DEFAULT_DESC_NULLS);
         } else {
             return sortType;
         }

h2/src/main/org/h2/table/IndexColumn.java

@@ -36,14 +36,7 @@ public class IndexColumn {
      */
     public String getSQL() {
         StringBuilder buff = new StringBuilder(column.getSQL());
-        if ((sortType & SortOrder.DESCENDING) != 0) {
-            buff.append(" DESC");
-        }
-        if ((sortType & SortOrder.NULLS_FIRST) != 0) {
-            buff.append(" NULLS FIRST");
-        } else if ((sortType & SortOrder.NULLS_LAST) != 0) {
-            buff.append(" NULLS LAST");
-        }
+        SortOrder.typeToString(buff, sortType);
         return buff.toString();
     }
 

h2/src/main/org/h2/util/StatementBuilder.java

@@ -127,4 +127,12 @@ public class StatementBuilder {
         return builder.length();
     }
 
+    /**
+     * Return underlying builder.
+     * @return underlying builder
+     */
+    public StringBuilder builder() {
+        return builder;
+    }
+
 }