MONDRIAN: Change dialect to recognize that databases don't tend to so…

…rt nulls first

    or nulls last; they tend to treat nulls as negative infinity or positive infinity.
    In Dialect, replace isNullsCollateLast() with getNullCollation(), and
    replace forceNullsCollateLast() with generateOrderItem().

[git-p4: depot-paths = "//open/mondrian/": change = 12467]

src/main/mondrian/rolap/sql/SqlQuery.java

@@ -23,7 +23,6 @@
 import javax.sql.DataSource;
 import java.io.PrintWriter;
 import java.io.StringWriter;
-import java.sql.*;
 import java.util.*;
 
 /**
@@ -439,19 +438,11 @@ public void addOrderBy(
         boolean prepend,
         boolean nullable)
     {
-        if (nullable && !dialect.isNullsCollateLast()) {
-            expr = dialect.forceNullsCollateLast(expr);
-        }
-
-        if (ascending) {
-            expr = expr + " ASC";
-        } else {
-            expr = expr + " DESC";
-        }
+        String orderExpr = dialect.generateOrderItem(expr, nullable, ascending);
         if (prepend) {
-            orderBy.add(0, expr);
+            orderBy.add(0, orderExpr);
         } else {
-            orderBy.add(expr);
+            orderBy.add(orderExpr);
         }
     }
 

src/main/mondrian/spi/Dialect.java

@@ -406,22 +406,45 @@ void quote(
     boolean allowsDdl();
 
     /**
-     * Returns whether NULL values appear last when sorted using ORDER BY.
-     * According to the SQL standard, this is implementation-specific.
+     * Returns the rule which determines whether NULL values appear first or
+     * last when sorted using ORDER BY.
      *
-     * @return Whether NULL values collate last
+     * <p>According to the SQL standard, this is implementation-specific.
+     * The default behavior is
+     * {@link mondrian.spi.Dialect.NullCollation#POSINF}.
+     *
+     * @return Rule which determines whether NULL values collate first or last
      */
-    boolean isNullsCollateLast();
+    NullCollation getNullCollation();
 
     /**
-     * Modifies an expression in the ORDER BY clause to ensure that NULL
-     * values collate after all non-NULL values.
-     * If {@link #isNullsCollateLast()} is true, there's nothing to do.
+     * Generates an item for an ORDER BY clause, sorting in the required
+     * direction, and ensuring that NULL values collate after all non-NULL
+     * values.
+     *
+     * <p>By default, {@code generateOrderItem(expr, true)} generates "expr ASC"
+     * and {@code generateOrderItem(expr, false)} generates "expr DESC". But
+     * depending on {@link #getNullCollation()} and {@code ascending}, there
+     * may need to be additional code.
+     *
+     * <p>For example, on Oracle, where NULLs collate higher than all other
+     * values, {@code generateOrderItem(expr, true)} generates "expr ASC" and
+     * {@code generateOrderItem(expr, false)} generates "expr DESC NULLS LAST".
+     *
+     * <p>On MySQL, where NULLs collate lower than all other values,
+     * {@code generateOrderItem(expr, true)} generates "ISNULL(expr), expr ASC"
+     * and {@code generateOrderItem(expr, false)} generates "expr DESC".
      *
      * @param expr Expression
+     * @param nullable Whether expression may have NULL values
+     * @param ascending Whether to sort expression ascending
+     *
      * @return Expression modified so that NULL values collate last
      */
-    String forceNullsCollateLast(String expr);
+    String generateOrderItem(
+        String expr,
+        boolean nullable,
+        boolean ascending);
 
     /**
      * Returns whether this Dialect supports expressions in the GROUP BY
@@ -728,6 +751,27 @@ public boolean isNumeric() {
             return false;
         }
     }
+
+    /**
+     * Description of how {@code NULL} values are ordered in an {@code ORDER BY}
+     * clause.
+     *
+     * <p>Values such as {@code FIRST}, {@code LAST}, and {@code BEFORE_ZERO}
+     * are possible, but no known database that has these behaviors.
+     */
+    enum NullCollation {
+        /**
+         * Null values order as negative infinity.
+         * They appear first with ASC, last with DESC.
+         */
+        NEGINF,
+
+        /**
+         * Null values order as positive infinity.
+         * They appear last with ASC, first with DESC.
+         */
+        POSINF;
+    }
 }
 
 // End Dialect.java

src/main/mondrian/spi/impl/InfobrightDialect.java

@@ -44,11 +44,20 @@ public boolean allowsCompoundCountDistinct() {
         return false;
     }
 
-    public boolean isNullsCollateLast() {
-        // Infobright is similar to MySQL, but apparently NULLs collate
-        // last. This is good news, because the workaround that we use on MySQL
-        // to force NULLs to collate last would kill Infobright's performance.an
-        return true;
+    public String generateOrderItem(
+        String expr,
+        boolean nullable,
+        boolean ascending)
+    {
+        // Like MySQL, Infobright collates NULL values as negative-infinity
+        // (first in ASC, last in DESC). But we can't generate ISNULL to
+        // correct the NULL ordering, as we do for MySQL, because Infobright
+        // does not support this function.
+        if (ascending) {
+            return expr + " ASC";
+        } else {
+            return expr + " DESC";
+        }
     }
 
     public boolean supportsGroupByExpressions() {

src/main/mondrian/spi/impl/JdbcDialectImpl.java

@@ -653,14 +653,43 @@ public boolean allowsDdl() {
         return !readOnly;
     }
 
-    public boolean isNullsCollateLast() {
-        return true;
+    public NullCollation getNullCollation() {
+        return NullCollation.POSINF;
     }
 
-    public String forceNullsCollateLast(String expr) {
-        // If we need to support other DBMSes, note that the SQL standard
-        // provides the syntax 'ORDER BY x ASC NULLS LAST'.
-        return expr;
+    public String generateOrderItem(
+        String expr,
+        boolean nullable,
+        boolean ascending)
+    {
+        if (nullable) {
+            NullCollation collateLast = getNullCollation();
+            switch (collateLast) {
+            case NEGINF:
+                // For DESC, NULLs already appear last.
+                // For ASC, we need to reverse the order.
+                // Use the SQL standard syntax 'ORDER BY x ASC NULLS LAST'.
+                if (ascending) {
+                    return expr + " ASC NULLS LAST";
+                } else {
+                    return expr + " DESC";
+                }
+            case POSINF:
+                if (ascending) {
+                    return expr + " ASC";
+                } else {
+                    return expr + " DESC NULLS LAST";
+                }
+            default:
+                throw Util.unexpected(collateLast);
+            }
+        } else {
+            if (ascending) {
+                return expr + " ASC";
+            } else {
+                return expr + " DESC";
+            }
+        }
     }
 
     public boolean supportsGroupByExpressions() {

src/main/mondrian/spi/impl/MySqlDialect.java

@@ -146,14 +146,29 @@ public String generateInline(
             columnNames, columnTypes, valueList, null, false);
     }
 
-    public boolean isNullsCollateLast() {
-        return false;
+    public NullCollation getNullCollation() {
+        return NullCollation.NEGINF;
     }
 
-    public String forceNullsCollateLast(String expr) {
-        String addIsNull = "ISNULL(" + expr + "), ";
-        expr = addIsNull + expr;
-        return expr;
+    public String generateOrderItem(
+        String expr,
+        boolean nullable,
+        boolean ascending)
+    {
+        if (nullable) {
+            assert getNullCollation() == NullCollation.NEGINF;
+            if (ascending) {
+                return "ISNULL(" + expr + "), " + expr;
+            } else {
+                return expr + " DESC";
+            }
+        } else {
+            if (ascending) {
+                return expr + " ASC";
+            } else {
+                return expr + " DESC";
+            }
+        }
     }
 
     public boolean requiresOrderByAlias() {

testsrc/main/mondrian/test/DialectTest.java

@@ -48,11 +48,17 @@
 public class DialectTest extends TestCase {
     private Connection connection;
     private Dialect dialect;
-    private static final
-    String
-        INFOBRIGHT_UNSUPPORTED =
-        "The query includes syntax that is not supported by the Infobright Optimizer. Either restructure the query with supported syntax, or enable the MySQL Query Path in the brighthouse.ini file to execute the query with reduced performance.";
+    private static final String INFOBRIGHT_UNSUPPORTED =
+        "The query includes syntax that is not supported by the Infobright"
+        + " Optimizer. Either restructure the query with supported syntax, or"
+        + " enable the MySQL Query Path in the brighthouse.ini file to execute"
+        + " the query with reduced performance.";
 
+    /**
+     * Creates a DialectTest.
+     *
+     * @param name Test case name
+     */
     public DialectTest(String name) {
         super(name);
     }
@@ -85,7 +91,7 @@ protected Dialect getDialect() {
     protected Connection getConnection() {
         if (connection == null) {
             try {
-            connection = getDataSource().getConnection();
+                connection = getDataSource().getConnection();
             } catch (SQLException e) {
                 throw Util.newInternal(e, "while creating connection");
             }
@@ -436,6 +442,56 @@ public void testGenerateInline() throws SQLException {
             new String[]{"a", "2008-04-29"}, new String[]{"b", "2007-01-02"});
     }
 
+    /**
+     * Tests that the method {@link mondrian.spi.Dialect#getNullCollation()}
+     * is accurate.
+     */
+    public void testNullCollation() throws SQLException {
+        Dialect dialect = getDialect();
+        String ascQuery =
+            "select "
+            + dialect.quoteIdentifier("grocery_sqft")
+            + " from "
+            + dialect.quoteIdentifier("store")
+            + " order by "
+            + dialect.quoteIdentifier("grocery_sqft");
+        String descQuery = ascQuery + " DESC";
+        Dialect.NullCollation nullCollation = getDialect().getNullCollation();
+        switch (nullCollation) {
+        case NEGINF:
+            assertFirstLast(ascQuery, null, 30351);
+            assertFirstLast(descQuery, 30351, null);
+            break;
+        case POSINF:
+            assertFirstLast(ascQuery, 13305, null);
+            assertFirstLast(descQuery, null, 13305);
+            break;
+        default:
+            fail("unexpected value " + nullCollation);
+        }
+    }
+
+    private void assertFirstLast(
+        String query,
+        Integer expectedFirst,
+        Integer expectedLast) throws SQLException
+    {
+        ResultSet resultSet =
+            getConnection().createStatement().executeQuery(query);
+        List<Integer> values = new ArrayList<Integer>();
+        while (resultSet.next()) {
+            values.add(resultSet.getInt(1));
+            if (resultSet.wasNull()) {
+                values.set(values.size() - 1, null);
+            }
+        }
+        resultSet.close();
+        Integer actualFirst = values.get(0);
+        Integer actualLast = values.get(values.size() - 1);
+        assertEquals(expectedFirst, actualFirst);
+        assertEquals(expectedLast, actualLast);
+    }
+
     private void assertInline(
         List<String> nameList,
         List<String> typeList,