This is an automated email from the ASF dual-hosted git repository. fjy pushed a commit to branch 0.20.0 in repository https://gitbox.apache.org/repos/asf/druid.git
The following commit(s) were added to refs/heads/0.20.0 by this push: new 239e9f0 fix array types from escaping into wider query engine (#10460) (#10474) 239e9f0 is described below commit 239e9f006e021a9c2ba81107a0c65071a0c7a59d Author: Jonathan Wei <jon-...@users.noreply.github.com> AuthorDate: Sat Oct 3 19:50:21 2020 -0700 fix array types from escaping into wider query engine (#10460) (#10474) * fix array types from escaping into wider query engine * oops * adjust * fix lgtm Co-authored-by: Clint Wylie <cwy...@apache.org> --- .../apache/druid/math/expr/BinaryOperatorExpr.java | 8 +- .../org/apache/druid/math/expr/ConstantExpr.java | 11 ++ .../main/java/org/apache/druid/math/expr/Expr.java | 6 + .../java/org/apache/druid/math/expr/ExprType.java | 104 -------------- .../apache/druid/math/expr/ExprTypeConversion.java | 159 +++++++++++++++++++++ .../java/org/apache/druid/math/expr/Function.java | 33 +++-- .../org/apache/druid/math/expr/OutputTypeTest.java | 142 ++++++++++-------- .../druid/segment/virtual/ExpressionPlanner.java | 12 +- .../segment/virtual/ExpressionVirtualColumn.java | 10 +- .../druid/sql/calcite/expression/Expressions.java | 17 --- .../builtin/ReductionOperatorConversionHelper.java | 3 +- .../apache/druid/sql/calcite/planner/Calcites.java | 9 +- .../apache/druid/sql/calcite/rel/QueryMaker.java | 16 ++- .../apache/druid/sql/calcite/CalciteQueryTest.java | 30 +++- 14 files changed, 354 insertions(+), 206 deletions(-) diff --git a/core/src/main/java/org/apache/druid/math/expr/BinaryOperatorExpr.java b/core/src/main/java/org/apache/druid/math/expr/BinaryOperatorExpr.java index 128a780..20ecc5d 100644 --- a/core/src/main/java/org/apache/druid/math/expr/BinaryOperatorExpr.java +++ b/core/src/main/java/org/apache/druid/math/expr/BinaryOperatorExpr.java @@ -83,7 +83,13 @@ abstract class BinaryOpExprBase implements Expr @Override public ExprType getOutputType(InputBindingTypes inputTypes) { - return ExprType.operatorAutoTypeConversion(left.getOutputType(inputTypes), right.getOutputType(inputTypes)); + if (left.isNullLiteral()) { + return right.getOutputType(inputTypes); + } + if (right.isNullLiteral()) { + return left.getOutputType(inputTypes); + } + return ExprTypeConversion.operator(left.getOutputType(inputTypes), right.getOutputType(inputTypes)); } @Override diff --git a/core/src/main/java/org/apache/druid/math/expr/ConstantExpr.java b/core/src/main/java/org/apache/druid/math/expr/ConstantExpr.java index 279600d..57ae900 100644 --- a/core/src/main/java/org/apache/druid/math/expr/ConstantExpr.java +++ b/core/src/main/java/org/apache/druid/math/expr/ConstantExpr.java @@ -99,6 +99,11 @@ abstract class NullNumericConstantExpr extends ConstantExpr } + @Override + public boolean isNullLiteral() + { + return true; + } } class LongExpr extends ConstantExpr @@ -429,6 +434,12 @@ class StringExpr extends ConstantExpr } @Override + public boolean isNullLiteral() + { + return value == null; + } + + @Override public String toString() { return value; diff --git a/core/src/main/java/org/apache/druid/math/expr/Expr.java b/core/src/main/java/org/apache/druid/math/expr/Expr.java index be0a32e..ff646fe 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Expr.java +++ b/core/src/main/java/org/apache/druid/math/expr/Expr.java @@ -53,6 +53,12 @@ public interface Expr return false; } + default boolean isNullLiteral() + { + // Overridden by things that are null literals. + return false; + } + /** * Returns the value of expr if expr is a literal, or throws an exception otherwise. * diff --git a/core/src/main/java/org/apache/druid/math/expr/ExprType.java b/core/src/main/java/org/apache/druid/math/expr/ExprType.java index e11b8ace..ebdf64a 100644 --- a/core/src/main/java/org/apache/druid/math/expr/ExprType.java +++ b/core/src/main/java/org/apache/druid/math/expr/ExprType.java @@ -19,7 +19,6 @@ package org.apache.druid.math.expr; -import org.apache.druid.java.util.common.IAE; import org.apache.druid.java.util.common.ISE; import org.apache.druid.segment.column.ValueType; @@ -169,107 +168,4 @@ public enum ExprType return elementType; } - /** - * Given 2 'input' types, choose the most appropriate combined type, if possible - * - * arrays must be the same type - * if both types are {@link #STRING}, the output type will be preserved as string - * if both types are {@link #LONG}, the output type will be preserved as long - * - */ - @Nullable - public static ExprType operatorAutoTypeConversion(@Nullable ExprType type, @Nullable ExprType other) - { - if (type == null || other == null) { - // cannot auto conversion unknown types - return null; - } - // arrays cannot be auto converted - if (isArray(type) || isArray(other)) { - if (!type.equals(other)) { - throw new IAE("Cannot implicitly cast %s to %s", type, other); - } - return type; - } - // if both arguments are a string, type becomes a string - if (STRING.equals(type) && STRING.equals(other)) { - return STRING; - } - - // otherwise a decimal or integer number - return numericAutoTypeConversion(type, other); - } - - /** - * Given 2 'input' types, choose the most appropriate combined type, if possible - * - * arrays must be the same type - * if either type is {@link #STRING}, the output type will be preserved as string - * if both types are {@link #LONG}, the output type will be preserved as long, otherwise {@link #DOUBLE} - */ - @Nullable - public static ExprType doubleMathFunctionAutoTypeConversion(@Nullable ExprType type, @Nullable ExprType other) - { - if (type == null || other == null) { - // cannot auto conversion unknown types - return null; - } - // arrays cannot be auto converted - if (isArray(type) || isArray(other)) { - if (!type.equals(other)) { - throw new IAE("Cannot implicitly cast %s to %s", type, other); - } - return type; - } - // if either argument is a string, type becomes a string - if (STRING.equals(type) || STRING.equals(other)) { - return STRING; - } - - return numericAutoTypeConversion(type, other); - } - - /** - * Given 2 'input' types, choose the most appropriate combined type, if possible - * - * arrays must be the same type - * if either type is {@link #STRING}, the output type will be preserved as string - * any number will be coerced to {@link #LONG} - */ - @Nullable - public static ExprType integerMathFunctionAutoTypeConversion(@Nullable ExprType type, @Nullable ExprType other) - { - if (type == null || other == null) { - // cannot auto conversion unknown types - return null; - } - // arrays cannot be auto converted - if (isArray(type) || isArray(other)) { - if (!type.equals(other)) { - throw new IAE("Cannot implicitly cast %s to %s", type, other); - } - return type; - } - // if either argument is a string, type becomes a string - if (STRING.equals(type) || STRING.equals(other)) { - return STRING; - } - - // any number is long - return LONG; - } - - /** - * Default best effort numeric type conversion. If both types are {@link #LONG}, returns {@link #LONG}, else - * {@link #DOUBLE} - */ - public static ExprType numericAutoTypeConversion(ExprType type, ExprType other) - { - // all numbers win over longs - if (LONG.equals(type) && LONG.equals(other)) { - return LONG; - } - // floats vs doubles would be handled here, but we currently only support doubles... - return DOUBLE; - } } diff --git a/core/src/main/java/org/apache/druid/math/expr/ExprTypeConversion.java b/core/src/main/java/org/apache/druid/math/expr/ExprTypeConversion.java new file mode 100644 index 0000000..2fc4577 --- /dev/null +++ b/core/src/main/java/org/apache/druid/math/expr/ExprTypeConversion.java @@ -0,0 +1,159 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.druid.math.expr; + +import org.apache.druid.java.util.common.IAE; + +import javax.annotation.Nullable; +import java.util.List; + +public class ExprTypeConversion +{ + + /** + * Infer the output type of a list of possible 'conditional' expression outputs (where any of these could be the + * output expression if the corresponding case matching expression evaluates to true) + */ + static ExprType conditional(Expr.InputBindingTypes inputTypes, List<Expr> args) + { + ExprType type = null; + for (Expr arg : args) { + if (arg.isNullLiteral()) { + continue; + } + if (type == null) { + type = arg.getOutputType(inputTypes); + } else { + type = doubleMathFunction(type, arg.getOutputType(inputTypes)); + } + } + return type; + } + + /** + * Given 2 'input' types, choose the most appropriate combined type, if possible + * + * arrays must be the same type + * if both types are {@link ExprType#STRING}, the output type will be preserved as string + * if both types are {@link ExprType#LONG}, the output type will be preserved as long + * + */ + @Nullable + public static ExprType operator(@Nullable ExprType type, @Nullable ExprType other) + { + if (type == null || other == null) { + // cannot auto conversion unknown types + return null; + } + // arrays cannot be auto converted + if (ExprType.isArray(type) || ExprType.isArray(other)) { + if (!type.equals(other)) { + throw new IAE("Cannot implicitly cast %s to %s", type, other); + } + return type; + } + // if both arguments are a string, type becomes a string + if (ExprType.STRING.equals(type) && ExprType.STRING.equals(other)) { + return ExprType.STRING; + } + + // otherwise a decimal or integer number + return numeric(type, other); + } + + /** + * Given 2 'input' types, choose the most appropriate combined type, if possible + * + * arrays must be the same type + * if either type is {@link ExprType#STRING}, the output type will be preserved as string + * if both types are {@link ExprType#LONG}, the output type will be preserved as long, otherwise + * {@link ExprType#DOUBLE} + */ + @Nullable + public static ExprType doubleMathFunction(@Nullable ExprType type, @Nullable ExprType other) + { + if (type == null || other == null) { + // cannot auto conversion unknown types + return null; + } + // arrays cannot be auto converted + if (ExprType.isArray(type) || ExprType.isArray(other)) { + if (!type.equals(other)) { + throw new IAE("Cannot implicitly cast %s to %s", type, other); + } + return type; + } + // if either argument is a string, type becomes a string + if (ExprType.STRING.equals(type) || ExprType.STRING.equals(other)) { + return ExprType.STRING; + } + + return numeric(type, other); + } + + /** + * Given 2 'input' types, choose the most appropriate combined type, if possible + * + * arrays must be the same type + * if either type is {@link ExprType#STRING}, the output type will be preserved as string + * any number will be coerced to {@link ExprType#LONG} + */ + @Nullable + public static ExprType integerMathFunction(@Nullable ExprType type, @Nullable ExprType other) + { + if (type == null || other == null) { + // cannot auto conversion unknown types + return null; + } + // arrays cannot be auto converted + if (ExprType.isArray(type) || ExprType.isArray(other)) { + if (!type.equals(other)) { + throw new IAE("Cannot implicitly cast %s to %s", type, other); + } + return type; + } + // if either argument is a string, type becomes a string + if (ExprType.STRING.equals(type) || ExprType.STRING.equals(other)) { + return ExprType.STRING; + } + + // any number is long + return ExprType.LONG; + } + + /** + * Default best effort numeric type conversion. If both types are {@link ExprType#LONG}, returns + * {@link ExprType#LONG}, else {@link ExprType#DOUBLE} + */ + public static ExprType numeric(ExprType type, ExprType other) + { + // all numbers win over longs + if (ExprType.LONG.equals(type) && ExprType.LONG.equals(other)) { + return ExprType.LONG; + } + // floats vs doubles would be handled here, but we currently only support doubles... + return ExprType.DOUBLE; + } + + private ExprTypeConversion() + { + // no instantiation + } +} diff --git a/core/src/main/java/org/apache/druid/math/expr/Function.java b/core/src/main/java/org/apache/druid/math/expr/Function.java index d812fa8..488a35c 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Function.java +++ b/core/src/main/java/org/apache/druid/math/expr/Function.java @@ -279,7 +279,7 @@ public interface Function @Override public ExprType getOutputType(Expr.InputBindingTypes inputTypes, List<Expr> args) { - return ExprType.doubleMathFunctionAutoTypeConversion(args.get(0).getOutputType(inputTypes), args.get(1).getOutputType(inputTypes)); + return ExprTypeConversion.doubleMathFunction(args.get(0).getOutputType(inputTypes), args.get(1).getOutputType(inputTypes)); } @Override @@ -444,7 +444,7 @@ public interface Function { ExprType outputType = ExprType.LONG; for (Expr expr : args) { - outputType = ExprType.doubleMathFunctionAutoTypeConversion(outputType, expr.getOutputType(inputTypes)); + outputType = ExprTypeConversion.doubleMathFunction(outputType, expr.getOutputType(inputTypes)); } return outputType; } @@ -465,7 +465,7 @@ public interface Function ExprType exprType = exprEval.type(); if (isValidType(exprType)) { - outputType = ExprType.doubleMathFunctionAutoTypeConversion(outputType, exprType); + outputType = ExprTypeConversion.doubleMathFunction(outputType, exprType); } if (exprEval.value() != null) { @@ -843,7 +843,7 @@ public interface Function @Override public ExprType getOutputType(Expr.InputBindingTypes inputTypes, List<Expr> args) { - return ExprType.integerMathFunctionAutoTypeConversion(args.get(0).getOutputType(inputTypes), args.get(1).getOutputType(inputTypes)); + return ExprTypeConversion.integerMathFunction(args.get(0).getOutputType(inputTypes), args.get(1).getOutputType(inputTypes)); } @Override @@ -1700,8 +1700,7 @@ public interface Function @Override public ExprType getOutputType(Expr.InputBindingTypes inputTypes, List<Expr> args) { - // output type is defined by else - return args.get(2).getOutputType(inputTypes); + return ExprTypeConversion.conditional(inputTypes, args.subList(1, 3)); } } @@ -1744,8 +1743,13 @@ public interface Function @Override public ExprType getOutputType(Expr.InputBindingTypes inputTypes, List<Expr> args) { - // output type is defined by else - return args.get(args.size() - 1).getOutputType(inputTypes); + List<Expr> results = new ArrayList<>(); + for (int i = 1; i < args.size(); i += 2) { + results.add(args.get(i)); + } + // add else + results.add(args.get(args.size() - 1)); + return ExprTypeConversion.conditional(inputTypes, results); } } @@ -1788,8 +1792,13 @@ public interface Function @Override public ExprType getOutputType(Expr.InputBindingTypes inputTypes, List<Expr> args) { - // output type is defined by else - return args.get(args.size() - 1).getOutputType(inputTypes); + List<Expr> results = new ArrayList<>(); + for (int i = 2; i < args.size(); i += 2) { + results.add(args.get(i)); + } + // add else + results.add(args.get(args.size() - 1)); + return ExprTypeConversion.conditional(inputTypes, results); } } @@ -1820,7 +1829,7 @@ public interface Function @Override public ExprType getOutputType(Expr.InputBindingTypes inputTypes, List<Expr> args) { - return args.get(0).getOutputType(inputTypes); + return ExprTypeConversion.conditional(inputTypes, args); } } @@ -2619,7 +2628,7 @@ public interface Function { ExprType type = ExprType.LONG; for (Expr arg : args) { - type = ExprType.doubleMathFunctionAutoTypeConversion(type, arg.getOutputType(inputTypes)); + type = ExprTypeConversion.doubleMathFunction(type, arg.getOutputType(inputTypes)); } return ExprType.asArrayType(type); } diff --git a/core/src/test/java/org/apache/druid/math/expr/OutputTypeTest.java b/core/src/test/java/org/apache/druid/math/expr/OutputTypeTest.java index c28fdb8..441a421 100644 --- a/core/src/test/java/org/apache/druid/math/expr/OutputTypeTest.java +++ b/core/src/test/java/org/apache/druid/math/expr/OutputTypeTest.java @@ -195,6 +195,17 @@ public class OutputTypeTest extends InitializedNullHandlingTest ); assertOutputType( + "case_simple(y,2,2,3,3.0,4)", + inputTypes, + ExprType.DOUBLE + ); + assertOutputType( + "case_simple(z,2.0,2.0,3.0,3.0,null)", + inputTypes, + ExprType.DOUBLE + ); + + assertOutputType( "case_searched(x=='baz','is baz',x=='foo','is foo','is other')", inputTypes, ExprType.STRING @@ -209,10 +220,27 @@ public class OutputTypeTest extends InitializedNullHandlingTest inputTypes, ExprType.DOUBLE ); + assertOutputType( + "case_searched(y==1,1,y==2,2.0,0)", + inputTypes, + ExprType.DOUBLE + ); + assertOutputType( + "case_searched(z==1.0,1,z==2.0,2,null)", + inputTypes, + ExprType.LONG + ); + assertOutputType( + "case_searched(z==1.0,1.0,z==2.0,2.0,null)", + inputTypes, + ExprType.DOUBLE + ); assertOutputType("nvl(x, 'foo')", inputTypes, ExprType.STRING); assertOutputType("nvl(y, 1)", inputTypes, ExprType.LONG); + assertOutputType("nvl(y, 1.1)", inputTypes, ExprType.DOUBLE); assertOutputType("nvl(z, 2.0)", inputTypes, ExprType.DOUBLE); + assertOutputType("nvl(y, 2.0)", inputTypes, ExprType.DOUBLE); assertOutputType("isnull(x)", inputTypes, ExprType.LONG); assertOutputType("isnull(y)", inputTypes, ExprType.LONG); assertOutputType("isnull(z)", inputTypes, ExprType.LONG); @@ -365,33 +393,33 @@ public class OutputTypeTest extends InitializedNullHandlingTest public void testOperatorAutoConversion() { // nulls output nulls - Assert.assertNull(ExprType.operatorAutoTypeConversion(ExprType.LONG, null)); - Assert.assertNull(ExprType.operatorAutoTypeConversion(null, ExprType.LONG)); - Assert.assertNull(ExprType.operatorAutoTypeConversion(ExprType.DOUBLE, null)); - Assert.assertNull(ExprType.operatorAutoTypeConversion(null, ExprType.DOUBLE)); - Assert.assertNull(ExprType.operatorAutoTypeConversion(ExprType.STRING, null)); - Assert.assertNull(ExprType.operatorAutoTypeConversion(null, ExprType.STRING)); + Assert.assertNull(ExprTypeConversion.operator(ExprType.LONG, null)); + Assert.assertNull(ExprTypeConversion.operator(null, ExprType.LONG)); + Assert.assertNull(ExprTypeConversion.operator(ExprType.DOUBLE, null)); + Assert.assertNull(ExprTypeConversion.operator(null, ExprType.DOUBLE)); + Assert.assertNull(ExprTypeConversion.operator(ExprType.STRING, null)); + Assert.assertNull(ExprTypeConversion.operator(null, ExprType.STRING)); // only long stays long - Assert.assertEquals(ExprType.LONG, ExprType.operatorAutoTypeConversion(ExprType.LONG, ExprType.LONG)); + Assert.assertEquals(ExprType.LONG, ExprTypeConversion.operator(ExprType.LONG, ExprType.LONG)); // only string stays string - Assert.assertEquals(ExprType.STRING, ExprType.operatorAutoTypeConversion(ExprType.STRING, ExprType.STRING)); + Assert.assertEquals(ExprType.STRING, ExprTypeConversion.operator(ExprType.STRING, ExprType.STRING)); // for operators, doubles is the catch all - Assert.assertEquals(ExprType.DOUBLE, ExprType.operatorAutoTypeConversion(ExprType.LONG, ExprType.DOUBLE)); - Assert.assertEquals(ExprType.DOUBLE, ExprType.operatorAutoTypeConversion(ExprType.DOUBLE, ExprType.LONG)); - Assert.assertEquals(ExprType.DOUBLE, ExprType.operatorAutoTypeConversion(ExprType.DOUBLE, ExprType.DOUBLE)); - Assert.assertEquals(ExprType.DOUBLE, ExprType.operatorAutoTypeConversion(ExprType.DOUBLE, ExprType.STRING)); - Assert.assertEquals(ExprType.DOUBLE, ExprType.operatorAutoTypeConversion(ExprType.STRING, ExprType.DOUBLE)); - Assert.assertEquals(ExprType.DOUBLE, ExprType.operatorAutoTypeConversion(ExprType.STRING, ExprType.LONG)); - Assert.assertEquals(ExprType.DOUBLE, ExprType.operatorAutoTypeConversion(ExprType.LONG, ExprType.STRING)); + Assert.assertEquals(ExprType.DOUBLE, ExprTypeConversion.operator(ExprType.LONG, ExprType.DOUBLE)); + Assert.assertEquals(ExprType.DOUBLE, ExprTypeConversion.operator(ExprType.DOUBLE, ExprType.LONG)); + Assert.assertEquals(ExprType.DOUBLE, ExprTypeConversion.operator(ExprType.DOUBLE, ExprType.DOUBLE)); + Assert.assertEquals(ExprType.DOUBLE, ExprTypeConversion.operator(ExprType.DOUBLE, ExprType.STRING)); + Assert.assertEquals(ExprType.DOUBLE, ExprTypeConversion.operator(ExprType.STRING, ExprType.DOUBLE)); + Assert.assertEquals(ExprType.DOUBLE, ExprTypeConversion.operator(ExprType.STRING, ExprType.LONG)); + Assert.assertEquals(ExprType.DOUBLE, ExprTypeConversion.operator(ExprType.LONG, ExprType.STRING)); // unless it is an array, and those have to be the same - Assert.assertEquals(ExprType.LONG_ARRAY, ExprType.operatorAutoTypeConversion(ExprType.LONG_ARRAY, ExprType.LONG_ARRAY)); + Assert.assertEquals(ExprType.LONG_ARRAY, ExprTypeConversion.operator(ExprType.LONG_ARRAY, ExprType.LONG_ARRAY)); Assert.assertEquals( ExprType.DOUBLE_ARRAY, - ExprType.operatorAutoTypeConversion(ExprType.DOUBLE_ARRAY, ExprType.DOUBLE_ARRAY) + ExprTypeConversion.operator(ExprType.DOUBLE_ARRAY, ExprType.DOUBLE_ARRAY) ); Assert.assertEquals( ExprType.STRING_ARRAY, - ExprType.operatorAutoTypeConversion(ExprType.STRING_ARRAY, ExprType.STRING_ARRAY) + ExprTypeConversion.operator(ExprType.STRING_ARRAY, ExprType.STRING_ARRAY) ); } @@ -399,33 +427,33 @@ public class OutputTypeTest extends InitializedNullHandlingTest public void testFunctionAutoConversion() { // nulls output nulls - Assert.assertNull(ExprType.doubleMathFunctionAutoTypeConversion(ExprType.LONG, null)); - Assert.assertNull(ExprType.doubleMathFunctionAutoTypeConversion(null, ExprType.LONG)); - Assert.assertNull(ExprType.doubleMathFunctionAutoTypeConversion(ExprType.DOUBLE, null)); - Assert.assertNull(ExprType.doubleMathFunctionAutoTypeConversion(null, ExprType.DOUBLE)); - Assert.assertNull(ExprType.doubleMathFunctionAutoTypeConversion(ExprType.STRING, null)); - Assert.assertNull(ExprType.doubleMathFunctionAutoTypeConversion(null, ExprType.STRING)); + Assert.assertNull(ExprTypeConversion.doubleMathFunction(ExprType.LONG, null)); + Assert.assertNull(ExprTypeConversion.doubleMathFunction(null, ExprType.LONG)); + Assert.assertNull(ExprTypeConversion.doubleMathFunction(ExprType.DOUBLE, null)); + Assert.assertNull(ExprTypeConversion.doubleMathFunction(null, ExprType.DOUBLE)); + Assert.assertNull(ExprTypeConversion.doubleMathFunction(ExprType.STRING, null)); + Assert.assertNull(ExprTypeConversion.doubleMathFunction(null, ExprType.STRING)); // only long stays long - Assert.assertEquals(ExprType.LONG, ExprType.doubleMathFunctionAutoTypeConversion(ExprType.LONG, ExprType.LONG)); + Assert.assertEquals(ExprType.LONG, ExprTypeConversion.doubleMathFunction(ExprType.LONG, ExprType.LONG)); // any double makes all doubles - Assert.assertEquals(ExprType.DOUBLE, ExprType.doubleMathFunctionAutoTypeConversion(ExprType.LONG, ExprType.DOUBLE)); - Assert.assertEquals(ExprType.DOUBLE, ExprType.doubleMathFunctionAutoTypeConversion(ExprType.DOUBLE, ExprType.LONG)); - Assert.assertEquals(ExprType.DOUBLE, ExprType.doubleMathFunctionAutoTypeConversion(ExprType.DOUBLE, ExprType.DOUBLE)); + Assert.assertEquals(ExprType.DOUBLE, ExprTypeConversion.doubleMathFunction(ExprType.LONG, ExprType.DOUBLE)); + Assert.assertEquals(ExprType.DOUBLE, ExprTypeConversion.doubleMathFunction(ExprType.DOUBLE, ExprType.LONG)); + Assert.assertEquals(ExprType.DOUBLE, ExprTypeConversion.doubleMathFunction(ExprType.DOUBLE, ExprType.DOUBLE)); // any string makes become string - Assert.assertEquals(ExprType.STRING, ExprType.doubleMathFunctionAutoTypeConversion(ExprType.LONG, ExprType.STRING)); - Assert.assertEquals(ExprType.STRING, ExprType.doubleMathFunctionAutoTypeConversion(ExprType.STRING, ExprType.LONG)); - Assert.assertEquals(ExprType.STRING, ExprType.doubleMathFunctionAutoTypeConversion(ExprType.DOUBLE, ExprType.STRING)); - Assert.assertEquals(ExprType.STRING, ExprType.doubleMathFunctionAutoTypeConversion(ExprType.STRING, ExprType.DOUBLE)); - Assert.assertEquals(ExprType.STRING, ExprType.doubleMathFunctionAutoTypeConversion(ExprType.STRING, ExprType.STRING)); + Assert.assertEquals(ExprType.STRING, ExprTypeConversion.doubleMathFunction(ExprType.LONG, ExprType.STRING)); + Assert.assertEquals(ExprType.STRING, ExprTypeConversion.doubleMathFunction(ExprType.STRING, ExprType.LONG)); + Assert.assertEquals(ExprType.STRING, ExprTypeConversion.doubleMathFunction(ExprType.DOUBLE, ExprType.STRING)); + Assert.assertEquals(ExprType.STRING, ExprTypeConversion.doubleMathFunction(ExprType.STRING, ExprType.DOUBLE)); + Assert.assertEquals(ExprType.STRING, ExprTypeConversion.doubleMathFunction(ExprType.STRING, ExprType.STRING)); // unless it is an array, and those have to be the same - Assert.assertEquals(ExprType.LONG_ARRAY, ExprType.doubleMathFunctionAutoTypeConversion(ExprType.LONG_ARRAY, ExprType.LONG_ARRAY)); + Assert.assertEquals(ExprType.LONG_ARRAY, ExprTypeConversion.doubleMathFunction(ExprType.LONG_ARRAY, ExprType.LONG_ARRAY)); Assert.assertEquals( ExprType.DOUBLE_ARRAY, - ExprType.doubleMathFunctionAutoTypeConversion(ExprType.DOUBLE_ARRAY, ExprType.DOUBLE_ARRAY) + ExprTypeConversion.doubleMathFunction(ExprType.DOUBLE_ARRAY, ExprType.DOUBLE_ARRAY) ); Assert.assertEquals( ExprType.STRING_ARRAY, - ExprType.doubleMathFunctionAutoTypeConversion(ExprType.STRING_ARRAY, ExprType.STRING_ARRAY) + ExprTypeConversion.doubleMathFunction(ExprType.STRING_ARRAY, ExprType.STRING_ARRAY) ); } @@ -433,32 +461,32 @@ public class OutputTypeTest extends InitializedNullHandlingTest public void testIntegerFunctionAutoConversion() { // nulls output nulls - Assert.assertNull(ExprType.integerMathFunctionAutoTypeConversion(ExprType.LONG, null)); - Assert.assertNull(ExprType.integerMathFunctionAutoTypeConversion(null, ExprType.LONG)); - Assert.assertNull(ExprType.integerMathFunctionAutoTypeConversion(ExprType.DOUBLE, null)); - Assert.assertNull(ExprType.integerMathFunctionAutoTypeConversion(null, ExprType.DOUBLE)); - Assert.assertNull(ExprType.integerMathFunctionAutoTypeConversion(ExprType.STRING, null)); - Assert.assertNull(ExprType.integerMathFunctionAutoTypeConversion(null, ExprType.STRING)); + Assert.assertNull(ExprTypeConversion.integerMathFunction(ExprType.LONG, null)); + Assert.assertNull(ExprTypeConversion.integerMathFunction(null, ExprType.LONG)); + Assert.assertNull(ExprTypeConversion.integerMathFunction(ExprType.DOUBLE, null)); + Assert.assertNull(ExprTypeConversion.integerMathFunction(null, ExprType.DOUBLE)); + Assert.assertNull(ExprTypeConversion.integerMathFunction(ExprType.STRING, null)); + Assert.assertNull(ExprTypeConversion.integerMathFunction(null, ExprType.STRING)); // all numbers are longs - Assert.assertEquals(ExprType.LONG, ExprType.integerMathFunctionAutoTypeConversion(ExprType.LONG, ExprType.LONG)); - Assert.assertEquals(ExprType.LONG, ExprType.integerMathFunctionAutoTypeConversion(ExprType.LONG, ExprType.DOUBLE)); - Assert.assertEquals(ExprType.LONG, ExprType.integerMathFunctionAutoTypeConversion(ExprType.DOUBLE, ExprType.LONG)); - Assert.assertEquals(ExprType.LONG, ExprType.integerMathFunctionAutoTypeConversion(ExprType.DOUBLE, ExprType.DOUBLE)); + Assert.assertEquals(ExprType.LONG, ExprTypeConversion.integerMathFunction(ExprType.LONG, ExprType.LONG)); + Assert.assertEquals(ExprType.LONG, ExprTypeConversion.integerMathFunction(ExprType.LONG, ExprType.DOUBLE)); + Assert.assertEquals(ExprType.LONG, ExprTypeConversion.integerMathFunction(ExprType.DOUBLE, ExprType.LONG)); + Assert.assertEquals(ExprType.LONG, ExprTypeConversion.integerMathFunction(ExprType.DOUBLE, ExprType.DOUBLE)); // any string makes become string - Assert.assertEquals(ExprType.STRING, ExprType.integerMathFunctionAutoTypeConversion(ExprType.LONG, ExprType.STRING)); - Assert.assertEquals(ExprType.STRING, ExprType.integerMathFunctionAutoTypeConversion(ExprType.STRING, ExprType.LONG)); - Assert.assertEquals(ExprType.STRING, ExprType.integerMathFunctionAutoTypeConversion(ExprType.DOUBLE, ExprType.STRING)); - Assert.assertEquals(ExprType.STRING, ExprType.integerMathFunctionAutoTypeConversion(ExprType.STRING, ExprType.DOUBLE)); - Assert.assertEquals(ExprType.STRING, ExprType.integerMathFunctionAutoTypeConversion(ExprType.STRING, ExprType.STRING)); + Assert.assertEquals(ExprType.STRING, ExprTypeConversion.integerMathFunction(ExprType.LONG, ExprType.STRING)); + Assert.assertEquals(ExprType.STRING, ExprTypeConversion.integerMathFunction(ExprType.STRING, ExprType.LONG)); + Assert.assertEquals(ExprType.STRING, ExprTypeConversion.integerMathFunction(ExprType.DOUBLE, ExprType.STRING)); + Assert.assertEquals(ExprType.STRING, ExprTypeConversion.integerMathFunction(ExprType.STRING, ExprType.DOUBLE)); + Assert.assertEquals(ExprType.STRING, ExprTypeConversion.integerMathFunction(ExprType.STRING, ExprType.STRING)); // unless it is an array - Assert.assertEquals(ExprType.LONG_ARRAY, ExprType.integerMathFunctionAutoTypeConversion(ExprType.LONG_ARRAY, ExprType.LONG_ARRAY)); + Assert.assertEquals(ExprType.LONG_ARRAY, ExprTypeConversion.integerMathFunction(ExprType.LONG_ARRAY, ExprType.LONG_ARRAY)); Assert.assertEquals( ExprType.DOUBLE_ARRAY, - ExprType.integerMathFunctionAutoTypeConversion(ExprType.DOUBLE_ARRAY, ExprType.DOUBLE_ARRAY) + ExprTypeConversion.integerMathFunction(ExprType.DOUBLE_ARRAY, ExprType.DOUBLE_ARRAY) ); Assert.assertEquals( ExprType.STRING_ARRAY, - ExprType.integerMathFunctionAutoTypeConversion(ExprType.STRING_ARRAY, ExprType.STRING_ARRAY) + ExprTypeConversion.integerMathFunction(ExprType.STRING_ARRAY, ExprType.STRING_ARRAY) ); } @@ -466,21 +494,21 @@ public class OutputTypeTest extends InitializedNullHandlingTest public void testAutoConversionArrayMismatchArrays() { expectedException.expect(IAE.class); - ExprType.doubleMathFunctionAutoTypeConversion(ExprType.DOUBLE_ARRAY, ExprType.LONG_ARRAY); + ExprTypeConversion.doubleMathFunction(ExprType.DOUBLE_ARRAY, ExprType.LONG_ARRAY); } @Test public void testAutoConversionArrayMismatchArrayScalar() { expectedException.expect(IAE.class); - ExprType.doubleMathFunctionAutoTypeConversion(ExprType.DOUBLE_ARRAY, ExprType.LONG); + ExprTypeConversion.doubleMathFunction(ExprType.DOUBLE_ARRAY, ExprType.LONG); } @Test public void testAutoConversionArrayMismatchScalarArray() { expectedException.expect(IAE.class); - ExprType.doubleMathFunctionAutoTypeConversion(ExprType.STRING, ExprType.LONG_ARRAY); + ExprTypeConversion.doubleMathFunction(ExprType.STRING, ExprType.LONG_ARRAY); } private void assertOutputType(String expression, Expr.InputBindingTypes inputTypes, ExprType outputType) diff --git a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionPlanner.java b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionPlanner.java index dc6361e..45122f0 100644 --- a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionPlanner.java +++ b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionPlanner.java @@ -150,8 +150,16 @@ public class ExpressionPlanner outputType = expression.getOutputType(inspector); } - // if analysis, inferred output type, or implicit mapping is in play, output will be multi-valued - if (analysis.isOutputArray() || ExprType.isArray(outputType) || ExpressionPlan.is(traits, ExpressionPlan.Trait.NEEDS_APPLIED)) { + // if analysis predicts output, or inferred output type is array, output will be multi-valued + if (analysis.isOutputArray() || ExprType.isArray(outputType)) { + traits.add(ExpressionPlan.Trait.NON_SCALAR_OUTPUT); + + // single input mappable may not produce array output explicitly, only through implicit mapping + traits.remove(ExpressionPlan.Trait.SINGLE_INPUT_MAPPABLE); + } + + // if implicit mapping is in play, output will be multi-valued but may still use SINGLE_INPUT_MAPPABLE optimization + if (ExpressionPlan.is(traits, ExpressionPlan.Trait.NEEDS_APPLIED)) { traits.add(ExpressionPlan.Trait.NON_SCALAR_OUTPUT); } diff --git a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionVirtualColumn.java b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionVirtualColumn.java index 3260afa..4a7635c 100644 --- a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionVirtualColumn.java +++ b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionVirtualColumn.java @@ -175,6 +175,12 @@ public class ExpressionVirtualColumn implements VirtualColumn // are unable to compute the output type of the expression, either due to incomplete type information of the // inputs or because of unimplemented methods on expression implementations themselves, or, because a // ColumnInspector is not available + + // array types must not currently escape from the expression system + if (outputType != null && outputType.isArray()) { + return new ColumnCapabilitiesImpl().setType(ValueType.STRING).setHasMultipleValues(true); + } + return new ColumnCapabilitiesImpl().setType(outputType == null ? ValueType.FLOAT : outputType); } @@ -185,7 +191,8 @@ public class ExpressionVirtualColumn implements VirtualColumn if (plan.getOutputType() != null) { - if (outputType != null && ExprType.fromValueType(outputType) != plan.getOutputType()) { + final ExprType inferredOutputType = plan.getOutputType(); + if (outputType != null && ExprType.fromValueType(outputType) != inferredOutputType) { log.warn( "Projected output type %s of expression %s does not match provided type %s", plan.getOutputType(), @@ -193,7 +200,6 @@ public class ExpressionVirtualColumn implements VirtualColumn outputType ); } - final ExprType inferredOutputType = plan.getOutputType(); final ValueType valueType = ExprType.toValueType(inferredOutputType); if (valueType.isNumeric()) { diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java index e456474..3ddddd9 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java @@ -37,7 +37,6 @@ import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.granularity.Granularity; import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprMacroTable; -import org.apache.druid.math.expr.ExprType; import org.apache.druid.math.expr.Parser; import org.apache.druid.query.aggregation.PostAggregator; import org.apache.druid.query.expression.TimestampFloorExprMacro; @@ -54,7 +53,6 @@ import org.apache.druid.query.ordering.StringComparators; import org.apache.druid.segment.VirtualColumn; import org.apache.druid.segment.column.ColumnHolder; import org.apache.druid.segment.column.RowSignature; -import org.apache.druid.segment.column.ValueType; import org.apache.druid.sql.calcite.filtration.BoundRefKey; import org.apache.druid.sql.calcite.filtration.Bounds; import org.apache.druid.sql.calcite.filtration.Filtration; @@ -666,21 +664,6 @@ public class Expressions : null; } - public static ExprType exprTypeForValueType(final ValueType valueType) - { - switch (valueType) { - case LONG: - return ExprType.LONG; - case FLOAT: - case DOUBLE: - return ExprType.DOUBLE; - case STRING: - return ExprType.STRING; - default: - throw new ISE("No ExprType for valueType[%s]", valueType); - } - } - /** * Converts an expression to a Granularity, if possible. This is possible if, and only if, the expression * is a timestamp_floor function on the __time column with literal parameters for period, origin, and timeZone. diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ReductionOperatorConversionHelper.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ReductionOperatorConversionHelper.java index 1356de9..3fa5f36 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ReductionOperatorConversionHelper.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ReductionOperatorConversionHelper.java @@ -24,6 +24,7 @@ import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.sql.type.SqlReturnTypeInference; import org.apache.calcite.sql.type.SqlTypeName; import org.apache.druid.java.util.common.IAE; +import org.apache.druid.math.expr.ExprTypeConversion; import org.apache.druid.segment.column.ValueType; import org.apache.druid.sql.calcite.planner.Calcites; @@ -38,7 +39,7 @@ class ReductionOperatorConversionHelper * https://dev.mysql.com/doc/refman/8.0/en/comparison-operators.html#function_least * * @see org.apache.druid.math.expr.Function.ReduceFunction#apply - * @see org.apache.druid.math.expr.ExprType#doubleMathFunctionAutoTypeConversion + * @see ExprTypeConversion#doubleMathFunction */ static final SqlReturnTypeInference TYPE_INFERENCE = opBinding -> { diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java index c8364fd..0f38dd7 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java @@ -142,12 +142,15 @@ public class Calcites } else if (sqlTypeName == SqlTypeName.ARRAY) { SqlTypeName componentType = type.getComponentType().getSqlTypeName(); if (isDoubleType(componentType)) { - return ValueType.DOUBLE_ARRAY; + // in the future return ValueType.DOUBLE_ARRAY; + return ValueType.STRING; } if (isLongType(componentType)) { - return ValueType.LONG_ARRAY; + // in the future we will return ValueType.LONG_ARRAY; + return ValueType.STRING; } - return ValueType.STRING_ARRAY; + // in the future we will return ValueType.STRING_ARRAY; + return ValueType.STRING; } else { return null; } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rel/QueryMaker.java b/sql/src/main/java/org/apache/druid/sql/calcite/rel/QueryMaker.java index f8c3dc2..5528d91 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rel/QueryMaker.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rel/QueryMaker.java @@ -307,11 +307,17 @@ public class QueryMaker coercedValue = value.getClass().getName(); } } else if (sqlType == SqlTypeName.ARRAY) { - try { - coercedValue = jsonMapper.writeValueAsString(value); - } - catch (IOException e) { - throw new RuntimeException(e); + if (value instanceof String) { + coercedValue = NullHandling.nullToEmptyIfNeeded((String) value); + } else if (value instanceof NlsString) { + coercedValue = ((NlsString) value).getValue(); + } else { + try { + coercedValue = jsonMapper.writeValueAsString(value); + } + catch (IOException e) { + throw new RuntimeException(e); + } } } else { throw new ISE("Cannot coerce[%s] to %s", value.getClass().getName(), sqlType); diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index c232aa8..a87c951 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -14684,7 +14684,7 @@ public class CalciteQueryTest extends BaseCalciteQueryTest newScanQueryBuilder() .dataSource(CalciteTests.DATASOURCE1) .intervals(querySegmentSpec(Filtration.eternity())) - .virtualColumns(expressionVirtualColumn("v0", "array(1,2)", ValueType.LONG_ARRAY)) + .virtualColumns(expressionVirtualColumn("v0", "array(1,2)", ValueType.STRING)) .columns("dim1", "v0") .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST) .limit(1) @@ -14698,6 +14698,32 @@ public class CalciteQueryTest extends BaseCalciteQueryTest } @Test + public void testGroupByArrayFromCase() throws Exception + { + cannotVectorize(); + testQuery( + "SELECT CASE WHEN dim4 = 'a' THEN ARRAY['foo','bar','baz'] END as mv_value, count(1) from numfoo GROUP BY 1", + ImmutableList.of( + GroupByQuery.builder() + .setDataSource(CalciteTests.DATASOURCE3) + .setInterval(querySegmentSpec(Filtration.eternity())) + .setVirtualColumns(expressionVirtualColumn("v0", "case_searched((\"dim4\" == 'a'),array('foo','bar','baz'),null)", ValueType.STRING)) + .setDimensions(new DefaultDimensionSpec("v0", "_d0")) + .setGranularity(Granularities.ALL) + .setAggregatorSpecs(new CountAggregatorFactory("a0")) + .setContext(QUERY_CONTEXT_DEFAULT) + .build() + ), + ImmutableList.of( + new Object[]{null, 3L}, + new Object[]{"bar", 3L}, + new Object[]{"baz", 3L}, + new Object[]{"foo", 3L} + ) + ); + } + + @Test public void testSelectNonConstantArrayExpressionFromTable() throws Exception { testQuery( @@ -14706,7 +14732,7 @@ public class CalciteQueryTest extends BaseCalciteQueryTest newScanQueryBuilder() .dataSource(CalciteTests.DATASOURCE1) .intervals(querySegmentSpec(Filtration.eternity())) - .virtualColumns(expressionVirtualColumn("v0", "array(concat(\"dim1\",'word'),'up')", ValueType.STRING_ARRAY)) + .virtualColumns(expressionVirtualColumn("v0", "array(concat(\"dim1\",'word'),'up')", ValueType.STRING)) .columns("dim1", "v0") .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST) .limit(5) --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org