fix a bugs related to SQL type inference return type nullability

core/src/main/java/org/apache/druid/math/expr/Function.java

@@ -2721,6 +2721,9 @@ public ExprType getOutputType(Expr.InputBindingInspector inspector, List<Expr> a
     @Override
     protected ExprEval eval(String x, int y)
     {
+      if (x == null) {
+        return ExprEval.of(null);
+      }
       return ExprEval.of(y < 1 ? NullHandling.defaultStringValue() : StringUtils.repeat(x, y));
     }
   }

core/src/test/java/org/apache/druid/math/expr/FunctionTest.java

@@ -596,6 +596,16 @@ public void testBitwise()
     assertExpr("bitwiseConvertDoubleToLongBits(null)", null);
   }
 
+  @Test
+  public void testRepeat()
+  {
+    assertExpr("repeat('hello', 2)", "hellohello");
+    assertExpr("repeat('hello', -1)", null);
+    assertExpr("repeat(null, 10)", null);
+    assertExpr("repeat(nonexistent, 10)", null);
+  }
+
+
   private void assertExpr(final String expression, @Nullable final Object expectedResult)
   {
     final Expr expr = Parser.parse(expression, ExprMacroTable.nil());

docs/querying/sql.md

@@ -303,7 +303,7 @@ columns in this mode are not nullable; any null or missing values will be treate
 In SQL compatible mode (`false`), NULLs are treated more closely to the SQL standard. The property affects both storage
 and querying, so for correct behavior, it should be set on all Druid service types to be available at both ingestion
 time and query time. There is some overhead associated with the ability to handle NULLs; see
-the [segment internals](../design/segments.md#sql-compatible-null-handling)documentation for more details.
+the [segment internals](../design/segments.md#sql-compatible-null-handling) documentation for more details.
 
 ## Aggregation functions
 

processing/src/main/java/org/apache/druid/query/expression/TimestampShiftExprMacro.java

@@ -99,7 +99,11 @@ private static class TimestampShiftExpr extends ExprMacroTable.BaseScalarMacroFu
     @Override
     public ExprEval eval(final ObjectBinding bindings)
     {
-      return ExprEval.of(chronology.add(period, args.get(0).eval(bindings).asLong(), step));
+      ExprEval timestamp = args.get(0).eval(bindings);
+      if (timestamp.isNumericNull()) {
+        return ExprEval.of(null);
+      }
+      return ExprEval.of(chronology.add(period, timestamp.asLong(), step));
     }
 
     @Override
@@ -128,10 +132,14 @@ private static class TimestampShiftDynamicExpr extends ExprMacroTable.BaseScalar
     @Override
     public ExprEval eval(final ObjectBinding bindings)
     {
+      ExprEval timestamp = args.get(0).eval(bindings);
+      if (timestamp.isNumericNull()) {
+        return ExprEval.of(null);
+      }
       final Period period = getPeriod(args, bindings);
       final Chronology chronology = getTimeZone(args, bindings);
       final int step = getStep(args, bindings);
-      return ExprEval.of(chronology.add(period, args.get(0).eval(bindings).asLong(), step));
+      return ExprEval.of(chronology.add(period, timestamp.asLong(), step));
     }
 
     @Override

processing/src/test/java/org/apache/druid/query/expression/TimestampShiftMacroTest.java

@@ -20,6 +20,7 @@
 package org.apache.druid.query.expression;
 
 import com.google.common.collect.ImmutableList;
+import org.apache.druid.common.config.NullHandling;
 import org.apache.druid.java.util.common.DateTimes;
 import org.apache.druid.java.util.common.IAE;
 import org.apache.druid.math.expr.Expr;
@@ -219,6 +220,24 @@ public Object get(String name)
     );
   }
 
+  @Test
+  public void testNull()
+  {
+    Expr expr = apply(
+        ImmutableList.of(
+            ExprEval.ofLong(null).toExpr(),
+            ExprEval.of("P1M").toExpr(),
+            ExprEval.of(1L).toExpr()
+        )
+    );
+
+    if (NullHandling.replaceWithDefault()) {
+      Assert.assertEquals(2678400000L, expr.eval(ExprUtils.nilBindings()).value());
+    } else {
+      Assert.assertNull(expr.eval(ExprUtils.nilBindings()).value());
+    }
+  }
+
   private static class NotLiteralExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctionExpr
   {
     NotLiteralExpr(String name)

sql/src/main/java/org/apache/druid/sql/calcite/expression/OperatorConversions.java

@@ -48,6 +48,7 @@
 import org.apache.calcite.sql.type.SqlReturnTypeInference;
 import org.apache.calcite.sql.type.SqlTypeFamily;
 import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.type.SqlTypeTransforms;
 import org.apache.calcite.util.Static;
 import org.apache.druid.java.util.common.IAE;
 import org.apache.druid.java.util.common.ISE;
@@ -255,11 +256,12 @@ private OperatorBuilder(final String name)
     }
 
     /**
-     * Sets the return type of the operator to "typeName", marked as non-nullable.
+     * Sets the return type of the operator to "typeName", marked as non-nullable. If this method is used it implies the
+     * operator should never, ever, return null.
      *
-     * One of {@link #returnTypeNonNull}, {@link #returnTypeNullable}, {@link #returnTypeNullableArray}, or
-     * {@link #returnTypeInference(SqlReturnTypeInference)} must be used before calling {@link #build()}. These methods
-     * cannot be mixed; you must call exactly one.
+     * One of {@link #returnTypeNonNull}, {@link #returnTypeNullable}, {@link #returnTypeCascadeNullable(SqlTypeName)}
+     * {@link #returnTypeNullableArray}, or {@link #returnTypeInference(SqlReturnTypeInference)} must be used before
+     * calling {@link #build()}. These methods cannot be mixed; you must call exactly one.
      */
     public OperatorBuilder returnTypeNonNull(final SqlTypeName typeName)
     {
@@ -274,9 +276,9 @@ public OperatorBuilder returnTypeNonNull(final SqlTypeName typeName)
     /**
      * Sets the return type of the operator to "typeName", marked as nullable.
      *
-     * One of {@link #returnTypeNonNull}, {@link #returnTypeNullable}, {@link #returnTypeNullableArray}, or
-     * {@link #returnTypeInference(SqlReturnTypeInference)} must be used before calling {@link #build()}. These methods
-     * cannot be mixed; you must call exactly one.
+     * One of {@link #returnTypeNonNull}, {@link #returnTypeNullable}, {@link #returnTypeCascadeNullable(SqlTypeName)}
+     * {@link #returnTypeNullableArray}, or {@link #returnTypeInference(SqlReturnTypeInference)} must be used before
+     * calling {@link #build()}. These methods cannot be mixed; you must call exactly one.
      */
     public OperatorBuilder returnTypeNullable(final SqlTypeName typeName)
     {
@@ -287,12 +289,27 @@ public OperatorBuilder returnTypeNullable(final SqlTypeName typeName)
       );
       return this;
     }
+
+    /**
+     * Sets the return type of the operator to "typeName", marked as nullable if any of its operands are nullable.
+     *
+     * One of {@link #returnTypeNonNull}, {@link #returnTypeNullable}, {@link #returnTypeCascadeNullable(SqlTypeName)}
+     * {@link #returnTypeNullableArray}, or {@link #returnTypeInference(SqlReturnTypeInference)} must be used before
+     * calling {@link #build()}. These methods cannot be mixed; you must call exactly one.
+     */
+    public OperatorBuilder returnTypeCascadeNullable(final SqlTypeName typeName)
+    {
+      Preconditions.checkState(this.returnTypeInference == null, "Cannot set return type multiple times");
+      this.returnTypeInference = ReturnTypes.cascade(ReturnTypes.explicit(typeName), SqlTypeTransforms.TO_NULLABLE);
+      return this;
+    }
+
     /**
      * Sets the return type of the operator to an array type with elements of "typeName", marked as nullable.
      *
-     * One of {@link #returnTypeNonNull}, {@link #returnTypeNullable}, {@link #returnTypeNullableArray}, or
-     * {@link #returnTypeInference(SqlReturnTypeInference)} must be used before calling {@link #build()}. These methods
-     * cannot be mixed; you must call exactly one.
+     * One of {@link #returnTypeNonNull}, {@link #returnTypeNullable}, {@link #returnTypeCascadeNullable(SqlTypeName)}
+     * {@link #returnTypeNullableArray}, or {@link #returnTypeInference(SqlReturnTypeInference)} must be used before
+     * calling {@link #build()}. These methods cannot be mixed; you must call exactly one.
      */
     public OperatorBuilder returnTypeNullableArray(final SqlTypeName elementTypeName)
     {
@@ -308,9 +325,9 @@ public OperatorBuilder returnTypeNullableArray(final SqlTypeName elementTypeName
     /**
      * Provides customized return type inference logic.
      *
-     * One of {@link #returnTypeNonNull}, {@link #returnTypeNullable}, {@link #returnTypeNullableArray}, or
-     * {@link #returnTypeInference(SqlReturnTypeInference)} must be used before calling {@link #build()}. These methods
-     * cannot be mixed; you must call exactly one.
+     * One of {@link #returnTypeNonNull}, {@link #returnTypeNullable}, {@link #returnTypeCascadeNullable(SqlTypeName)}
+     * {@link #returnTypeNullableArray}, or {@link #returnTypeInference(SqlReturnTypeInference)} must be used before
+     * calling {@link #build()}. These methods cannot be mixed; you must call exactly one.
      */
     public OperatorBuilder returnTypeInference(final SqlReturnTypeInference returnTypeInference)
     {

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayLengthOperatorConversion.java

@@ -43,7 +43,7 @@ public class ArrayLengthOperatorConversion implements SqlOperatorConversion
           )
       )
       .functionCategory(SqlFunctionCategory.STRING)
-      .returnTypeNonNull(SqlTypeName.INTEGER)
+      .returnTypeCascadeNullable(SqlTypeName.INTEGER)
       .build();
 
   @Override

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayOffsetOfOperatorConversion.java

@@ -47,7 +47,7 @@ public class ArrayOffsetOfOperatorConversion implements SqlOperatorConversion
           )
       )
       .functionCategory(SqlFunctionCategory.STRING)
-      .returnTypeNonNull(SqlTypeName.INTEGER)
+      .returnTypeNullable(SqlTypeName.INTEGER)
       .build();
 
   @Override

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayOrdinalOfOperatorConversion.java

@@ -47,7 +47,7 @@ public class ArrayOrdinalOfOperatorConversion implements SqlOperatorConversion
           )
       )
       .functionCategory(SqlFunctionCategory.STRING)
-      .returnTypeNonNull(SqlTypeName.INTEGER)
+      .returnTypeCascadeNullable(SqlTypeName.INTEGER)
       .build();
 
   @Override

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayToStringOperatorConversion.java

@@ -47,7 +47,7 @@ public class ArrayToStringOperatorConversion implements SqlOperatorConversion
           )
       )
       .functionCategory(SqlFunctionCategory.STRING)
-      .returnTypeNonNull(SqlTypeName.VARCHAR)
+      .returnTypeCascadeNullable(SqlTypeName.VARCHAR)
       .build();
 
   @Override

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/BTrimOperatorConversion.java

@@ -37,7 +37,7 @@ public class BTrimOperatorConversion implements SqlOperatorConversion
   private static final SqlFunction SQL_FUNCTION = OperatorConversions
       .operatorBuilder("BTRIM")
       .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.CHARACTER)
-      .returnTypeNonNull(SqlTypeName.VARCHAR)
+      .returnTypeCascadeNullable(SqlTypeName.VARCHAR)
       .functionCategory(SqlFunctionCategory.STRING)
       .requiredOperands(1)
       .build();

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ConcatOperatorConversion.java

@@ -22,29 +22,22 @@
 import org.apache.calcite.rex.RexNode;
 import org.apache.calcite.sql.SqlFunction;
 import org.apache.calcite.sql.SqlFunctionCategory;
-import org.apache.calcite.sql.SqlKind;
 import org.apache.calcite.sql.type.OperandTypes;
-import org.apache.calcite.sql.type.ReturnTypes;
 import org.apache.calcite.sql.type.SqlTypeName;
 import org.apache.druid.segment.column.RowSignature;
 import org.apache.druid.sql.calcite.expression.DruidExpression;
 import org.apache.druid.sql.calcite.expression.OperatorConversions;
 import org.apache.druid.sql.calcite.expression.SqlOperatorConversion;
-import org.apache.druid.sql.calcite.planner.Calcites;
 import org.apache.druid.sql.calcite.planner.PlannerContext;
 
 public class ConcatOperatorConversion implements SqlOperatorConversion
 {
-  private static final SqlFunction SQL_FUNCTION = new SqlFunction(
-      "CONCAT",
-      SqlKind.OTHER_FUNCTION,
-      ReturnTypes.explicit(
-          factory -> Calcites.createSqlType(factory, SqlTypeName.VARCHAR)
-      ),
-      null,
-      OperandTypes.SAME_VARIADIC,
-      SqlFunctionCategory.STRING
-  );
+  private static final SqlFunction SQL_FUNCTION = OperatorConversions
+      .operatorBuilder("CONCAT")
+      .operandTypeChecker(OperandTypes.SAME_VARIADIC)
+      .returnTypeCascadeNullable(SqlTypeName.VARCHAR)
+      .functionCategory(SqlFunctionCategory.STRING)
+      .build();
 
   @Override
   public SqlFunction calciteOperator()

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/DateTruncOperatorConversion.java

@@ -67,7 +67,7 @@ public class DateTruncOperatorConversion implements SqlOperatorConversion
       .operatorBuilder("DATE_TRUNC")
       .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.TIMESTAMP)
       .requiredOperands(2)
-      .returnTypeNonNull(SqlTypeName.TIMESTAMP)
+      .returnTypeCascadeNullable(SqlTypeName.TIMESTAMP)
       .functionCategory(SqlFunctionCategory.TIMEDATE)
       .build();
 

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/LPadOperatorConversion.java

@@ -37,7 +37,7 @@ public class LPadOperatorConversion implements SqlOperatorConversion
   private static final SqlFunction SQL_FUNCTION = OperatorConversions
       .operatorBuilder("LPAD")
       .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.INTEGER, SqlTypeFamily.CHARACTER)
-      .returnTypeNonNull(SqlTypeName.VARCHAR)
+      .returnTypeCascadeNullable(SqlTypeName.VARCHAR)
       .functionCategory(SqlFunctionCategory.STRING)
       .requiredOperands(2)
       .build();

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/LTrimOperatorConversion.java

@@ -37,7 +37,7 @@ public class LTrimOperatorConversion implements SqlOperatorConversion
   private static final SqlFunction SQL_FUNCTION = OperatorConversions
       .operatorBuilder("LTRIM")
       .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.CHARACTER)
-      .returnTypeNonNull(SqlTypeName.VARCHAR)
+      .returnTypeCascadeNullable(SqlTypeName.VARCHAR)
       .functionCategory(SqlFunctionCategory.STRING)
       .requiredOperands(1)
       .build();

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/LeftOperatorConversion.java

@@ -39,7 +39,7 @@ public class LeftOperatorConversion implements SqlOperatorConversion
       .operatorBuilder("LEFT")
       .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.INTEGER)
       .functionCategory(SqlFunctionCategory.STRING)
-      .returnTypeNonNull(SqlTypeName.VARCHAR)
+      .returnTypeCascadeNullable(SqlTypeName.VARCHAR)
       .build();
 
   @Override

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/MillisToTimestampOperatorConversion.java

@@ -39,7 +39,7 @@ public class MillisToTimestampOperatorConversion implements SqlOperatorConversio
   private static final SqlFunction SQL_FUNCTION = OperatorConversions
       .operatorBuilder("MILLIS_TO_TIMESTAMP")
       .operandTypes(SqlTypeFamily.EXACT_NUMERIC)
-      .returnTypeNonNull(SqlTypeName.TIMESTAMP)
+      .returnTypeCascadeNullable(SqlTypeName.TIMESTAMP)
       .functionCategory(SqlFunctionCategory.TIMEDATE)
       .build();
 

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ParseLongOperatorConversion.java

@@ -38,7 +38,7 @@ public class ParseLongOperatorConversion implements SqlOperatorConversion
   private static final SqlFunction SQL_FUNCTION = OperatorConversions
       .operatorBuilder(NAME)
       .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.INTEGER)
-      .returnTypeNonNull(SqlTypeName.BIGINT)
+      .returnTypeCascadeNullable(SqlTypeName.BIGINT)
       .functionCategory(SqlFunctionCategory.STRING)
       .requiredOperands(1)
       .build();

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/RPadOperatorConversion.java

@@ -37,7 +37,7 @@ public class RPadOperatorConversion implements SqlOperatorConversion
   private static final SqlFunction SQL_FUNCTION = OperatorConversions
       .operatorBuilder("RPAD")
       .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.INTEGER, SqlTypeFamily.CHARACTER)
-      .returnTypeNonNull(SqlTypeName.VARCHAR)
+      .returnTypeCascadeNullable(SqlTypeName.VARCHAR)
       .functionCategory(SqlFunctionCategory.STRING)
       .requiredOperands(2)
       .build();

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/RTrimOperatorConversion.java

@@ -37,7 +37,7 @@ public class RTrimOperatorConversion implements SqlOperatorConversion
   private static final SqlFunction SQL_FUNCTION = OperatorConversions
       .operatorBuilder("RTRIM")
       .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.CHARACTER)
-      .returnTypeNonNull(SqlTypeName.VARCHAR)
+      .returnTypeCascadeNullable(SqlTypeName.VARCHAR)
       .functionCategory(SqlFunctionCategory.STRING)
       .requiredOperands(1)
       .build();

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/RepeatOperatorConversion.java

@@ -39,7 +39,7 @@ public class RepeatOperatorConversion implements SqlOperatorConversion
       .operatorBuilder("REPEAT")
       .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.INTEGER)
       .functionCategory(SqlFunctionCategory.STRING)
-      .returnTypeNonNull(SqlTypeName.VARCHAR)
+      .returnTypeCascadeNullable(SqlTypeName.VARCHAR)
       .build();
 
   @Override

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ReverseOperatorConversion.java

@@ -37,7 +37,7 @@ public class ReverseOperatorConversion implements SqlOperatorConversion
       .operatorBuilder("REVERSE")
       .operandTypes(SqlTypeFamily.CHARACTER)
       .functionCategory(SqlFunctionCategory.STRING)
-      .returnTypeNonNull(SqlTypeName.VARCHAR)
+      .returnTypeCascadeNullable(SqlTypeName.VARCHAR)
       .build();
 
   @Override

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/RightOperatorConversion.java

@@ -39,7 +39,7 @@ public class RightOperatorConversion implements SqlOperatorConversion
       .operatorBuilder("RIGHT")
       .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.INTEGER)
       .functionCategory(SqlFunctionCategory.STRING)
-      .returnTypeNonNull(SqlTypeName.VARCHAR)
+      .returnTypeCascadeNullable(SqlTypeName.VARCHAR)
       .build();
 
   @Override

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/StringFormatOperatorConversion.java

@@ -42,7 +42,7 @@ public class StringFormatOperatorConversion implements SqlOperatorConversion
       .operatorBuilder("STRING_FORMAT")
       .operandTypeChecker(new StringFormatOperandTypeChecker())
       .functionCategory(SqlFunctionCategory.STRING)
-      .returnTypeNonNull(SqlTypeName.VARCHAR)
+      .returnTypeCascadeNullable(SqlTypeName.VARCHAR)
       .build();
 
   @Override

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/StrposOperatorConversion.java

@@ -38,7 +38,7 @@ public class StrposOperatorConversion implements SqlOperatorConversion
       .operatorBuilder("STRPOS")
       .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.CHARACTER)
       .functionCategory(SqlFunctionCategory.STRING)
-      .returnTypeNonNull(SqlTypeName.INTEGER)
+      .returnTypeCascadeNullable(SqlTypeName.INTEGER)
       .build();
 
   @Override

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TextcatOperatorConversion.java

@@ -36,7 +36,7 @@ public class TextcatOperatorConversion implements SqlOperatorConversion
       .operatorBuilder("textcat")
       .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.CHARACTER)
       .requiredOperands(2)
-      .returnTypeNonNull(SqlTypeName.VARCHAR)
+      .returnTypeCascadeNullable(SqlTypeName.VARCHAR)
       .functionCategory(SqlFunctionCategory.STRING)
       .build();
 

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TimeCeilOperatorConversion.java

@@ -41,7 +41,7 @@ public class TimeCeilOperatorConversion implements SqlOperatorConversion
       .operatorBuilder("TIME_CEIL")
       .operandTypes(SqlTypeFamily.TIMESTAMP, SqlTypeFamily.CHARACTER, SqlTypeFamily.TIMESTAMP, SqlTypeFamily.CHARACTER)
       .requiredOperands(2)
-      .returnTypeNonNull(SqlTypeName.TIMESTAMP)
+      .returnTypeCascadeNullable(SqlTypeName.TIMESTAMP)
       .functionCategory(SqlFunctionCategory.TIMEDATE)
       .build();