Make NULL handling in Druid more compatible to SQL standard

.travis.yml

@@ -50,8 +50,38 @@ matrix:
       install: echo "MAVEN_OPTS='-Xmx3000m'" > ~/.mavenrc && mvn install -q -ff -DskipTests -B
       before_script:
         - unset _JAVA_OPTIONS
+
       script: echo "MAVEN_OPTS='-Xmx512m'" > ~/.mavenrc && mvn test -B -Pparallel-test -Dmaven.fork.count=2 -pl '!processing,!server'
 
+      # processing module test with SQL Compatible Null Handling
+    - sudo: false
+      env:
+        - NAME="processing module test with SQL Compatible Null Handling"
+      install: echo "MAVEN_OPTS='-Xmx3000m'" > ~/.mavenrc && mvn install -q -ff -DskipTests -B
+      before_script:
+        - unset _JAVA_OPTIONS
+      script: echo "MAVEN_OPTS='-Xmx512m'" > ~/.mavenrc && mvn test -B -Pparallel-test -Dmaven.fork.count=2 -pl processing
+
+      # server module test with SQL Compatible Null Handling
+    - sudo: false
+      env:
+        - NAME="server module test with SQL Compatible Null Handling"
+      install: echo "MAVEN_OPTS='-Xmx3000m'" > ~/.mavenrc && mvn install -Ddruid.generic.useDefaultValueForNull=false -q -ff -DskipTests -B
+      before_script:
+        - unset _JAVA_OPTIONS
+      # Server module test is run without the parallel-test option because it's memory sensitive and often fails with that option.
+      script: echo "MAVEN_OPTS='-Xmx512m'" > ~/.mavenrc && mvn test -Ddruid.generic.useDefaultValueForNull=false -B -pl server
+
+      # other modules test with SQL Compatible Null Handling
+    - sudo: false
+      env:
+        - NAME="other modules test with SQL Compatible Null Handling"
+      install: echo "MAVEN_OPTS='-Xmx3000m'" > ~/.mavenrc && mvn install -q -ff -DskipTests -B
+      before_script:
+        - unset _JAVA_OPTIONS
+
+      script: echo "MAVEN_OPTS='-Xmx512m'" > ~/.mavenrc && mvn test -Ddruid.generic.useDefaultValueForNull=false -B -Pparallel-test -Dmaven.fork.count=2 -pl '!processing,!server'
+
       # run integration tests
     - sudo: required
       services:

api/src/main/java/io/druid/data/input/MapBasedRow.java

@@ -25,6 +25,7 @@
 import io.druid.java.util.common.DateTimes;
 import org.joda.time.DateTime;
 
+import javax.annotation.Nullable;
 import java.util.List;
 import java.util.Map;
 
@@ -80,6 +81,7 @@ public List<String> getDimension(String dimension)
   }
 
   @Override
+  @Nullable
   public Object getRaw(String dimension)
   {
     return event.get(dimension);

api/src/main/java/io/druid/data/input/Row.java

@@ -24,6 +24,7 @@
 import io.druid.guice.annotations.PublicApi;
 import org.joda.time.DateTime;
 
+import javax.annotation.Nullable;
 import java.util.List;
 
 /**
@@ -71,13 +72,13 @@ public interface Row extends Comparable<Row>
    *
    * @return the value of the provided column name
    */
+  @Nullable
   Object getRaw(String dimension);
 
   /**
-   * Returns the metric column value for the given column name. This method is different from {@link #getRaw} in two
-   * aspects:
-   *  1. If the column is absent in the row, numeric zero is returned, rather than null.
-   *  2. If the column has string value, an attempt is made to parse this value as a number.
+   * Returns the metric column value for the given column name. This method is different from {@link #getRaw}
+   * when column has string value, an attempt is made to parse this value as a number.
    */
+  @Nullable
   Number getMetric(String metric);
 }

api/src/main/java/io/druid/data/input/Rows.java

@@ -26,6 +26,7 @@
 import io.druid.java.util.common.StringUtils;
 import io.druid.java.util.common.parsers.ParseException;
 
+import javax.annotation.Nullable;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
@@ -92,10 +93,11 @@ public static List<String> objectToStrings(final Object inputValue)
    * @throws NullPointerException if the string is null
    * @throws ParseException       if the column cannot be converted to a number
    */
-  public static Number objectToNumber(final String name, final Object inputValue)
+  @Nullable
+  public static Number objectToNumber(final String name, @Nullable final Object inputValue)
   {
     if (inputValue == null) {
-      return Rows.LONG_ZERO;
+      return null;
     }
 
     if (inputValue instanceof Number) {

api/src/test/java/io/druid/data/input/MapBasedRowTest.java

@@ -41,13 +41,14 @@ public void testGetLongMetricFromString()
           .put("k6", "+9223372036854775802")
           .build()
     );
-    
+
     Assert.assertEquals(-1.2, row.getMetric("k0"));
     Assert.assertEquals(1.23, row.getMetric("k1"));
     Assert.assertEquals(1.8, row.getMetric("k2"));
     Assert.assertEquals(100000.0, row.getMetric("k3"));
     Assert.assertEquals(9223372036854775806L, row.getMetric("k4"));
     Assert.assertEquals(-9223372036854775807L, row.getMetric("k5"));
     Assert.assertEquals(9223372036854775802L, row.getMetric("k6"));
+    Assert.assertEquals(null, row.getMetric("k9"));
   }
 }

benchmarks/src/main/java/io/druid/benchmark/FilterPartitionBenchmark.java

@@ -610,8 +610,9 @@ public Filter toFilter()
       if (extractionFn == null) {
         return new NoBitmapSelectorFilter(dimension, value);
       } else {
+        //CHECKSTYLE.OFF: Regexp
         final String valueOrNull = Strings.emptyToNull(value);
-
+        //CHECKSTYLE.ON: Regexp
         final DruidPredicateFactory predicateFactory = new DruidPredicateFactory()
         {
           @Override

codestyle/checkstyle.xml

@@ -162,5 +162,15 @@
       <property name="illegalPattern" value="true"/>
       <property name="message" value="Use io.druid.java.util.common.logger.Logger instead"/>
     </module>
+    <module name="Regexp">
+      <property name="format" value="Strings.emptyToNull"/>
+      <property name="illegalPattern" value="true"/>
+      <property name="message" value="Use io.druid.common.config.NullHandling.emptyToNullIfNeeded instead"/>
+    </module>
+    <module name="Regexp">
+      <property name="format" value="Strings.nullToEmpty"/>
+      <property name="illegalPattern" value="true"/>
+      <property name="message" value="Use io.druid.common.config.NullHandling.nullToEmptyIfNeeded instead"/>
+    </module>
   </module>
 </module>

common/src/main/antlr4/io/druid/math/expr/antlr/Expr.g4

@@ -1,6 +1,7 @@
 grammar Expr;
 
-expr : ('-'|'!') expr                                 # unaryOpExpr
+expr : 'null'                                         # null
+     | ('-'|'!') expr                                 # unaryOpExpr
      |<assoc=right> expr '^' expr                     # powOpExpr
      | expr ('*'|'/'|'%') expr                        # mulDivModuloExpr
      | expr ('+'|'-') expr                            # addSubExpr

common/src/main/java/io/druid/common/config/NullHandling.java

@@ -0,0 +1,106 @@
+/*
+ * Licensed to Metamarkets Group Inc. (Metamarkets) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. Metamarkets licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package io.druid.common.config;
+
+import com.google.common.base.Strings;
+import com.google.inject.Inject;
+
+import javax.annotation.Nullable;
+
+/**
+ * Helper class for NullHandling. This class is used to switch between SQL compatible Null Handling behavior
+ * introduced as part of {@link https://github.com/druid-io/druid/issues/4349} and the old druid behavior
+ * where null values are replaced with default values e.g Null Strings are replaced with empty values.
+ */
+public class NullHandling
+{
+  private static String NULL_HANDLING_CONFIG_STRING = "druid.generic.useDefaultValueForNull";
+
+  /**
+   * use these values to ensure that {@link NullHandling#defaultDoubleValue()},
+   * {@link NullHandling#defaultFloatValue()} , {@link NullHandling#defaultFloatValue()}
+   * return the same boxed object when returning a constant zero
+   */
+  public static final Double ZERO_DOUBLE = 0.0d;
+  public static final Float ZERO_FLOAT = 0.0f;
+  public static final Long ZERO_LONG = 0L;
+
+  /**
+   * INSTANCE is injected using static injection to avoid adding JacksonInject annotations all over the code.
+   * See {@link io.druid.guice.NullHandlingModule} for details.
+   * It does not take effect in all unit tests since we don't use Guice Injection.
+   * For tests default system property is supposed to be used only in tests
+   */
+  @Inject
+  private static NullValueHandlingConfig INSTANCE = new NullValueHandlingConfig(
+      Boolean.valueOf(System.getProperty(NULL_HANDLING_CONFIG_STRING, "true"))
+  );
+
+  public static boolean useDefaultValuesForNull()
+  {
+    return INSTANCE.isUseDefaultValuesForNull();
+  }
+
+  @Nullable
+  public static String nullToEmptyIfNeeded(@Nullable String value)
+  {
+    //CHECKSTYLE.OFF: Regexp
+    return useDefaultValuesForNull() ? Strings.nullToEmpty(value) : value;
+    //CHECKSTYLE.ON: Regexp
+  }
+
+  @Nullable
+  public static String emptyToNullIfNeeded(@Nullable String value)
+  {
+    //CHECKSTYLE.OFF: Regexp
+    return useDefaultValuesForNull() ? Strings.emptyToNull(value) : value;
+    //CHECKSTYLE.ON: Regexp
+  }
+
+  @Nullable
+  public static String defaultStringValue()
+  {
+    return useDefaultValuesForNull() ? "" : null;
+  }
+
+  @Nullable
+  public static Long defaultLongValue()
+  {
+    return useDefaultValuesForNull() ? ZERO_LONG : null;
+  }
+
+  @Nullable
+  public static Float defaultFloatValue()
+  {
+    return useDefaultValuesForNull() ? ZERO_FLOAT : null;
+  }
+
+  @Nullable
+  public static Double defaultDoubleValue()
+  {
+    return useDefaultValuesForNull() ? ZERO_DOUBLE : null;
+  }
+
+  public static boolean isNullOrEquivalent(@Nullable String value)
+  {
+    return INSTANCE.isUseDefaultValuesForNull() ? Strings.isNullOrEmpty(value) : value == null;
+  }
+
+}

common/src/main/java/io/druid/common/config/NullValueHandlingConfig.java

@@ -0,0 +1,43 @@
+/*
+ * Licensed to Metamarkets Group Inc. (Metamarkets) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. Metamarkets licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package io.druid.common.config;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+public class NullValueHandlingConfig
+{
+
+  @JsonProperty("useDefaultValueForNull")
+  private final boolean useDefaultValuesForNull;
+
+  @JsonCreator
+  public NullValueHandlingConfig(@JsonProperty("useDefaultValueForNull") Boolean useDefaultValuesForNull)
+  {
+    this.useDefaultValuesForNull = useDefaultValuesForNull == null
+                                   ? true
+                                   : useDefaultValuesForNull;
+  }
+
+  public boolean isUseDefaultValuesForNull()
+  {
+    return useDefaultValuesForNull;
+  }
+}

common/src/main/java/io/druid/io/Channels.java

@@ -25,7 +25,6 @@
 
 public final class Channels
 {
-
   public static void writeFully(WritableByteChannel channel, ByteBuffer src) throws IOException
   {
     while (src.remaining() > 0) {

common/src/main/java/io/druid/math/expr/Expr.java

@@ -20,9 +20,9 @@
 package io.druid.math.expr;
 
 import com.google.common.base.Preconditions;
-import com.google.common.base.Strings;
 import com.google.common.math.LongMath;
 import com.google.common.primitives.Ints;
+import io.druid.common.config.NullHandling;
 import io.druid.java.util.common.IAE;
 import io.druid.java.util.common.ISE;
 import io.druid.java.util.common.guava.Comparators;
@@ -124,7 +124,7 @@ class StringExpr extends ConstantExpr
 
   public StringExpr(String value)
   {
-    this.value = Strings.emptyToNull(value);
+    this.value = NullHandling.emptyToNullIfNeeded(value);
   }
 
   @Nullable
@@ -362,6 +362,13 @@ public ExprEval eval(ObjectBinding bindings)
   {
     ExprEval leftVal = left.eval(bindings);
     ExprEval rightVal = right.eval(bindings);
+
+    // Result of any Binary expressions is null if any of the argument is null.
+    // e.g "select null * 2 as c;" or "select null + 1 as c;" will return null as per Standard SQL spec.
+    if (!NullHandling.useDefaultValuesForNull() && (leftVal.isNull() || rightVal.isNull())) {
+      return ExprEval.of(null);
+    }
+
     if (leftVal.type() == ExprType.STRING && rightVal.type() == ExprType.STRING) {
       return evalString(leftVal.asString(), rightVal.asString());
     } else if (leftVal.type() == ExprType.LONG && rightVal.type() == ExprType.LONG) {
@@ -491,7 +498,8 @@ class BinPlusExpr extends BinaryEvalOpExprBase
   @Override
   protected ExprEval evalString(@Nullable String left, @Nullable String right)
   {
-    return ExprEval.of(Strings.nullToEmpty(left) + Strings.nullToEmpty(right));
+    return ExprEval.of(NullHandling.nullToEmptyIfNeeded(left)
+                       + NullHandling.nullToEmptyIfNeeded(right));
   }
 
   @Override
@@ -698,3 +706,4 @@ public ExprEval eval(ObjectBinding bindings)
     return leftVal.asBoolean() ? leftVal : right.eval(bindings);
   }
 }
+