clintropolis commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r486756009
##########
File path: core/src/main/java/org/apache/druid/math/expr/ConstantExpr.java
##########
@@ -35,6 +35,19 @@
*/
abstract class ConstantExpr implements Expr
{
+ final ExprType outputType;
+
+ protected ConstantExpr(ExprType outputType)
+ {
+ this.outputType = outputType;
+ }
+
+ @Override
Review comment:
Oops, I didn't actually mean to make this not be `@Nullable`, though I
guess it is true, but will fix to just be consistent with the interface.
##########
File path: core/src/main/java/org/apache/druid/math/expr/Expr.java
##########
@@ -116,16 +116,39 @@ default String getBindingIfIdentifier()
void visit(Visitor visitor);
/**
- * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.Each
{@link Expr} is responsible for
+ * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.
Each {@link Expr} is responsible for
* ensuring the {@link Shuttle} can visit all of its {@link Expr} children,
as well as updating its children
* {@link Expr} with the results from the {@link Shuttle}, before finally
visiting an updated form of itself.
*/
Expr visit(Shuttle shuttle);
/**
- * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingDetails}
+ * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingAnalysis}
*/
- BindingDetails analyzeInputs();
+ BindingAnalysis analyzeInputs();
+
+ /**
+ * Given an {@link InputBindingTypes}, compute what the output {@link
ExprType} will be for this expression. A return
+ * value of null indicates that the given type information was not enough to
resolve the output type, so the
+ * expression must be evaluated using default {@link #eval} handling where
types are only known after evaluation,
+ * through {@link ExprEval#type}.
+ */
+ @Nullable
+ default ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return null;
+ }
+
+ /**
+ * Mechanism to supply input types for the bindings which will back {@link
IdentifierExpr}, to use in the aid of
+ * inferring the output type of an expression with {@link #getOutputType}. A
null value means that either the binding
Review comment:
not long term probably, but currently this will be backed by
`ColumnCapabilities` from a `ColumnInspector`, which does not currently
distinguish between those 2 things for all implementations.
##########
File path: core/src/main/java/org/apache/druid/math/expr/UnaryOperatorExpr.java
##########
@@ -163,4 +171,15 @@ public String toString()
{
return StringUtils.format("!%s", expr);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
Review comment:
I don't know off the top of my head why it does that, I was just trying
to model its output. The binary logic expressions do that too:
```
@Override
protected ExprEval evalString(@Nullable String left, @Nullable String
right)
{
return
ExprEval.ofLongBoolean(Comparators.<String>naturalNullsFirst().compare(left,
right) < 0);
}
```
so it is just being consistent with those operators I think.
I'm moderately in the camp of wondering why these logical operators return
anything other than long for any input type, but I haven't thought fully
through on the implications of changing that yet, so maybe there is a reason
that the type is preserved for doubles in these operators.
##########
File path:
processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java
##########
@@ -93,6 +95,13 @@ public Expr visit(Shuttle shuttle)
return shuttle.visit(new TimestampCeilExpr(newArgs));
}
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return ExprType.LONG;
Review comment:
Hmm, so numeric null handling should be done currently be done using
`ExprEval.isNumericNull`, which will return true even if the type is a null
string from a `StringExprEval`. So i think 'string' is technically correct, but
I don't think spiritually or functionally quite true, nor that useful. We
probably should modify these `ExprEval.of(null)` to make type correct
`ExprEval` to make `eval` match exactly what `getOutputType` says, but I don't
know if this PR is the correct place to do that.
##########
File path:
server/src/main/java/org/apache/druid/server/lookup/cache/LookupCoordinatorManager.java
##########
@@ -518,7 +518,7 @@ private void initializeLookupsConfigWatcher()
configManager.set(
LOOKUP_CONFIG_KEY,
converted,
- new AuditInfo("autoConversion", "autoConversion", "127.0.0.1")
+ new AuditInfo("autoTypeConversion", "autoTypeConversion",
"127.0.0.1")
Review comment:
oops, intellij got a bit aggro
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprType.java
##########
@@ -29,5 +35,116 @@
STRING,
DOUBLE_ARRAY,
LONG_ARRAY,
- STRING_ARRAY
+ STRING_ARRAY;
+
+ public boolean isNumeric()
+ {
+ return isNumeric(this);
+ }
+
+ /**
+ * The expression system does not distinguish between {@link
ValueType#FLOAT} and {@link ValueType#DOUBLE}, and
+ * cannot currently handle {@link ValueType#COMPLEX} inputs. This method
will convert {@link ValueType#FLOAT} to
+ * {@link #DOUBLE}, or throw an exception if a {@link ValueType#COMPLEX} is
encountered.
+ *
+ * @throws IllegalStateException
+ */
+ public static ExprType fromValueType(@Nullable ValueType valueType)
+ {
+ if (valueType == null) {
+ throw new IllegalStateException("Unsupported unknown value type");
+ }
+ switch (valueType) {
+ case LONG:
+ return LONG;
+ case LONG_ARRAY:
+ return LONG_ARRAY;
+ case FLOAT:
+ case DOUBLE:
+ return DOUBLE;
+ case DOUBLE_ARRAY:
+ return DOUBLE_ARRAY;
+ case STRING:
+ return STRING;
+ case STRING_ARRAY:
+ return STRING_ARRAY;
+ case COMPLEX:
+ default:
+ throw new ISE("Unsupported value type[%s]", valueType);
+ }
+ }
+
+ public static boolean isNumeric(ExprType type)
+ {
+ return LONG.equals(type) || DOUBLE.equals(type);
+ }
+
+ public static boolean isArray(@Nullable ExprType type)
+ {
+ return LONG_ARRAY.equals(type) || DOUBLE_ARRAY.equals(type) ||
STRING_ARRAY.equals(type);
+ }
+
+ @Nullable
+ public static ExprType elementType(@Nullable ExprType type)
+ {
+ if (type != null && isArray(type)) {
+ switch (type) {
+ case STRING_ARRAY:
+ return STRING;
+ case LONG_ARRAY:
+ return LONG;
+ case DOUBLE_ARRAY:
+ return DOUBLE;
+ }
+ }
+ return type;
+ }
+
+ @Nullable
+ public static ExprType asArrayType(@Nullable ExprType elementType)
+ {
+ if (elementType != null && !isArray(elementType)) {
+ switch (elementType) {
+ case STRING:
+ return STRING_ARRAY;
+ case LONG:
+ return LONG_ARRAY;
+ case DOUBLE:
+ return DOUBLE_ARRAY;
+ }
+ }
+ return elementType;
+ }
+
+ @Nullable
+ public static ExprType implicitCast(@Nullable ExprType type, @Nullable
ExprType other)
+ {
+ if (type == null || other == null) {
+ // cannot implicitly cast unknown types
+ return null;
+ }
+ // arrays cannot be implicitly cast
+ if (isArray(type)) {
+ 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;
+ }
+
+ if (isNumeric(type) && isNumeric(other)) {
+ // all numbers win over longs
+ if (LONG.equals(type)) {
+ return other;
+ }
+ // floats vs longs would be handled here, but we currently only support
doubles...
+ return type;
+ }
+
+ // unhandled is unknown
+ return null;
Review comment:
>As you said the code is volatile so chances of this path being hit are
even higher.
I mean, it's totally not possible to hit this null because there isn't a
`ExprType` combination of input arguments that ends up on this line. By
volatile I meant that this enum and file is likely going to go away and maybe
this function migrated into `ValueType`. This line is essentially a placeholder
for `ValueType.COMPLEX` which doesn't exist in `ExprType`, so I was shaping
this method to be straightforward to work off of that enum instead of this one
someday.
I can throw an exception with the messaging 'impossible' if you would
prefer, but there isn't a way to actually check it with a test.
##########
File path: core/src/main/java/org/apache/druid/math/expr/ConstantExpr.java
##########
@@ -35,6 +35,19 @@
*/
abstract class ConstantExpr implements Expr
{
+ final ExprType outputType;
+
+ protected ConstantExpr(ExprType outputType)
+ {
+ this.outputType = outputType;
+ }
+
+ @Override
Review comment:
Oops, I didn't actually mean to make this not be `@Nullable`, though I
guess it is true, but will fix to just be consistent with the interface.
##########
File path: core/src/main/java/org/apache/druid/math/expr/Expr.java
##########
@@ -116,16 +116,39 @@ default String getBindingIfIdentifier()
void visit(Visitor visitor);
/**
- * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.Each
{@link Expr} is responsible for
+ * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.
Each {@link Expr} is responsible for
* ensuring the {@link Shuttle} can visit all of its {@link Expr} children,
as well as updating its children
* {@link Expr} with the results from the {@link Shuttle}, before finally
visiting an updated form of itself.
*/
Expr visit(Shuttle shuttle);
/**
- * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingDetails}
+ * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingAnalysis}
*/
- BindingDetails analyzeInputs();
+ BindingAnalysis analyzeInputs();
+
+ /**
+ * Given an {@link InputBindingTypes}, compute what the output {@link
ExprType} will be for this expression. A return
+ * value of null indicates that the given type information was not enough to
resolve the output type, so the
+ * expression must be evaluated using default {@link #eval} handling where
types are only known after evaluation,
+ * through {@link ExprEval#type}.
+ */
+ @Nullable
+ default ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return null;
+ }
+
+ /**
+ * Mechanism to supply input types for the bindings which will back {@link
IdentifierExpr}, to use in the aid of
+ * inferring the output type of an expression with {@link #getOutputType}. A
null value means that either the binding
Review comment:
not long term probably, but currently this will be backed by
`ColumnCapabilities` from a `ColumnInspector`, which does not currently
distinguish between those 2 things for all implementations.
##########
File path: core/src/main/java/org/apache/druid/math/expr/UnaryOperatorExpr.java
##########
@@ -163,4 +171,15 @@ public String toString()
{
return StringUtils.format("!%s", expr);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
Review comment:
I don't know off the top of my head why it does that, I was just trying
to model its output. The binary logic expressions do that too:
```
@Override
protected ExprEval evalString(@Nullable String left, @Nullable String
right)
{
return
ExprEval.ofLongBoolean(Comparators.<String>naturalNullsFirst().compare(left,
right) < 0);
}
```
so it is just being consistent with those operators I think.
I'm moderately in the camp of wondering why these logical operators return
anything other than long for any input type, but I haven't thought fully
through on the implications of changing that yet, so maybe there is a reason
that the type is preserved for doubles in these operators.
##########
File path:
processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java
##########
@@ -93,6 +95,13 @@ public Expr visit(Shuttle shuttle)
return shuttle.visit(new TimestampCeilExpr(newArgs));
}
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return ExprType.LONG;
Review comment:
Hmm, so numeric null handling should be done currently be done using
`ExprEval.isNumericNull`, which will return true even if the type is a null
string from a `StringExprEval`. So i think 'string' is technically correct, but
I don't think spiritually or functionally quite true, nor that useful. We
probably should modify these `ExprEval.of(null)` to make type correct
`ExprEval` to make `eval` match exactly what `getOutputType` says, but I don't
know if this PR is the correct place to do that.
##########
File path:
server/src/main/java/org/apache/druid/server/lookup/cache/LookupCoordinatorManager.java
##########
@@ -518,7 +518,7 @@ private void initializeLookupsConfigWatcher()
configManager.set(
LOOKUP_CONFIG_KEY,
converted,
- new AuditInfo("autoConversion", "autoConversion", "127.0.0.1")
+ new AuditInfo("autoTypeConversion", "autoTypeConversion",
"127.0.0.1")
Review comment:
oops, intellij got a bit aggro
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprType.java
##########
@@ -29,5 +35,116 @@
STRING,
DOUBLE_ARRAY,
LONG_ARRAY,
- STRING_ARRAY
+ STRING_ARRAY;
+
+ public boolean isNumeric()
+ {
+ return isNumeric(this);
+ }
+
+ /**
+ * The expression system does not distinguish between {@link
ValueType#FLOAT} and {@link ValueType#DOUBLE}, and
+ * cannot currently handle {@link ValueType#COMPLEX} inputs. This method
will convert {@link ValueType#FLOAT} to
+ * {@link #DOUBLE}, or throw an exception if a {@link ValueType#COMPLEX} is
encountered.
+ *
+ * @throws IllegalStateException
+ */
+ public static ExprType fromValueType(@Nullable ValueType valueType)
+ {
+ if (valueType == null) {
+ throw new IllegalStateException("Unsupported unknown value type");
+ }
+ switch (valueType) {
+ case LONG:
+ return LONG;
+ case LONG_ARRAY:
+ return LONG_ARRAY;
+ case FLOAT:
+ case DOUBLE:
+ return DOUBLE;
+ case DOUBLE_ARRAY:
+ return DOUBLE_ARRAY;
+ case STRING:
+ return STRING;
+ case STRING_ARRAY:
+ return STRING_ARRAY;
+ case COMPLEX:
+ default:
+ throw new ISE("Unsupported value type[%s]", valueType);
+ }
+ }
+
+ public static boolean isNumeric(ExprType type)
+ {
+ return LONG.equals(type) || DOUBLE.equals(type);
+ }
+
+ public static boolean isArray(@Nullable ExprType type)
+ {
+ return LONG_ARRAY.equals(type) || DOUBLE_ARRAY.equals(type) ||
STRING_ARRAY.equals(type);
+ }
+
+ @Nullable
+ public static ExprType elementType(@Nullable ExprType type)
+ {
+ if (type != null && isArray(type)) {
+ switch (type) {
+ case STRING_ARRAY:
+ return STRING;
+ case LONG_ARRAY:
+ return LONG;
+ case DOUBLE_ARRAY:
+ return DOUBLE;
+ }
+ }
+ return type;
+ }
+
+ @Nullable
+ public static ExprType asArrayType(@Nullable ExprType elementType)
+ {
+ if (elementType != null && !isArray(elementType)) {
+ switch (elementType) {
+ case STRING:
+ return STRING_ARRAY;
+ case LONG:
+ return LONG_ARRAY;
+ case DOUBLE:
+ return DOUBLE_ARRAY;
+ }
+ }
+ return elementType;
+ }
+
+ @Nullable
+ public static ExprType implicitCast(@Nullable ExprType type, @Nullable
ExprType other)
+ {
+ if (type == null || other == null) {
+ // cannot implicitly cast unknown types
+ return null;
+ }
+ // arrays cannot be implicitly cast
+ if (isArray(type)) {
+ 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;
+ }
+
+ if (isNumeric(type) && isNumeric(other)) {
+ // all numbers win over longs
+ if (LONG.equals(type)) {
+ return other;
+ }
+ // floats vs longs would be handled here, but we currently only support
doubles...
+ return type;
+ }
+
+ // unhandled is unknown
+ return null;
Review comment:
>As you said the code is volatile so chances of this path being hit are
even higher.
I mean, it's totally not possible to hit this null because there isn't a
`ExprType` combination of input arguments that ends up on this line. By
volatile I meant that this enum and file is likely going to go away and maybe
this function migrated into `ValueType`. This line is essentially a placeholder
for `ValueType.COMPLEX` which doesn't exist in `ExprType`, so I was shaping
this method to be straightforward to work off of that enum instead of this one
someday.
I can throw an exception with the messaging 'impossible' if you would
prefer, but there isn't a way to actually check it with a test.
##########
File path: core/src/main/java/org/apache/druid/math/expr/ConstantExpr.java
##########
@@ -35,6 +35,19 @@
*/
abstract class ConstantExpr implements Expr
{
+ final ExprType outputType;
+
+ protected ConstantExpr(ExprType outputType)
+ {
+ this.outputType = outputType;
+ }
+
+ @Override
Review comment:
Oops, I didn't actually mean to make this not be `@Nullable`, though I
guess it is true, but will fix to just be consistent with the interface.
##########
File path: core/src/main/java/org/apache/druid/math/expr/Expr.java
##########
@@ -116,16 +116,39 @@ default String getBindingIfIdentifier()
void visit(Visitor visitor);
/**
- * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.Each
{@link Expr} is responsible for
+ * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.
Each {@link Expr} is responsible for
* ensuring the {@link Shuttle} can visit all of its {@link Expr} children,
as well as updating its children
* {@link Expr} with the results from the {@link Shuttle}, before finally
visiting an updated form of itself.
*/
Expr visit(Shuttle shuttle);
/**
- * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingDetails}
+ * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingAnalysis}
*/
- BindingDetails analyzeInputs();
+ BindingAnalysis analyzeInputs();
+
+ /**
+ * Given an {@link InputBindingTypes}, compute what the output {@link
ExprType} will be for this expression. A return
+ * value of null indicates that the given type information was not enough to
resolve the output type, so the
+ * expression must be evaluated using default {@link #eval} handling where
types are only known after evaluation,
+ * through {@link ExprEval#type}.
+ */
+ @Nullable
+ default ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return null;
+ }
+
+ /**
+ * Mechanism to supply input types for the bindings which will back {@link
IdentifierExpr}, to use in the aid of
+ * inferring the output type of an expression with {@link #getOutputType}. A
null value means that either the binding
Review comment:
not long term probably, but currently this will be backed by
`ColumnCapabilities` from a `ColumnInspector`, which does not currently
distinguish between those 2 things for all implementations.
##########
File path: core/src/main/java/org/apache/druid/math/expr/UnaryOperatorExpr.java
##########
@@ -163,4 +171,15 @@ public String toString()
{
return StringUtils.format("!%s", expr);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
Review comment:
I don't know off the top of my head why it does that, I was just trying
to model its output. The binary logic expressions do that too:
```
@Override
protected ExprEval evalString(@Nullable String left, @Nullable String
right)
{
return
ExprEval.ofLongBoolean(Comparators.<String>naturalNullsFirst().compare(left,
right) < 0);
}
```
so it is just being consistent with those operators I think.
I'm moderately in the camp of wondering why these logical operators return
anything other than long for any input type, but I haven't thought fully
through on the implications of changing that yet, so maybe there is a reason
that the type is preserved for doubles in these operators.
##########
File path:
processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java
##########
@@ -93,6 +95,13 @@ public Expr visit(Shuttle shuttle)
return shuttle.visit(new TimestampCeilExpr(newArgs));
}
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return ExprType.LONG;
Review comment:
Hmm, so numeric null handling should be done currently be done using
`ExprEval.isNumericNull`, which will return true even if the type is a null
string from a `StringExprEval`. So i think 'string' is technically correct, but
I don't think spiritually or functionally quite true, nor that useful. We
probably should modify these `ExprEval.of(null)` to make type correct
`ExprEval` to make `eval` match exactly what `getOutputType` says, but I don't
know if this PR is the correct place to do that.
##########
File path:
server/src/main/java/org/apache/druid/server/lookup/cache/LookupCoordinatorManager.java
##########
@@ -518,7 +518,7 @@ private void initializeLookupsConfigWatcher()
configManager.set(
LOOKUP_CONFIG_KEY,
converted,
- new AuditInfo("autoConversion", "autoConversion", "127.0.0.1")
+ new AuditInfo("autoTypeConversion", "autoTypeConversion",
"127.0.0.1")
Review comment:
oops, intellij got a bit aggro
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprType.java
##########
@@ -29,5 +35,116 @@
STRING,
DOUBLE_ARRAY,
LONG_ARRAY,
- STRING_ARRAY
+ STRING_ARRAY;
+
+ public boolean isNumeric()
+ {
+ return isNumeric(this);
+ }
+
+ /**
+ * The expression system does not distinguish between {@link
ValueType#FLOAT} and {@link ValueType#DOUBLE}, and
+ * cannot currently handle {@link ValueType#COMPLEX} inputs. This method
will convert {@link ValueType#FLOAT} to
+ * {@link #DOUBLE}, or throw an exception if a {@link ValueType#COMPLEX} is
encountered.
+ *
+ * @throws IllegalStateException
+ */
+ public static ExprType fromValueType(@Nullable ValueType valueType)
+ {
+ if (valueType == null) {
+ throw new IllegalStateException("Unsupported unknown value type");
+ }
+ switch (valueType) {
+ case LONG:
+ return LONG;
+ case LONG_ARRAY:
+ return LONG_ARRAY;
+ case FLOAT:
+ case DOUBLE:
+ return DOUBLE;
+ case DOUBLE_ARRAY:
+ return DOUBLE_ARRAY;
+ case STRING:
+ return STRING;
+ case STRING_ARRAY:
+ return STRING_ARRAY;
+ case COMPLEX:
+ default:
+ throw new ISE("Unsupported value type[%s]", valueType);
+ }
+ }
+
+ public static boolean isNumeric(ExprType type)
+ {
+ return LONG.equals(type) || DOUBLE.equals(type);
+ }
+
+ public static boolean isArray(@Nullable ExprType type)
+ {
+ return LONG_ARRAY.equals(type) || DOUBLE_ARRAY.equals(type) ||
STRING_ARRAY.equals(type);
+ }
+
+ @Nullable
+ public static ExprType elementType(@Nullable ExprType type)
+ {
+ if (type != null && isArray(type)) {
+ switch (type) {
+ case STRING_ARRAY:
+ return STRING;
+ case LONG_ARRAY:
+ return LONG;
+ case DOUBLE_ARRAY:
+ return DOUBLE;
+ }
+ }
+ return type;
+ }
+
+ @Nullable
+ public static ExprType asArrayType(@Nullable ExprType elementType)
+ {
+ if (elementType != null && !isArray(elementType)) {
+ switch (elementType) {
+ case STRING:
+ return STRING_ARRAY;
+ case LONG:
+ return LONG_ARRAY;
+ case DOUBLE:
+ return DOUBLE_ARRAY;
+ }
+ }
+ return elementType;
+ }
+
+ @Nullable
+ public static ExprType implicitCast(@Nullable ExprType type, @Nullable
ExprType other)
+ {
+ if (type == null || other == null) {
+ // cannot implicitly cast unknown types
+ return null;
+ }
+ // arrays cannot be implicitly cast
+ if (isArray(type)) {
+ 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;
+ }
+
+ if (isNumeric(type) && isNumeric(other)) {
+ // all numbers win over longs
+ if (LONG.equals(type)) {
+ return other;
+ }
+ // floats vs longs would be handled here, but we currently only support
doubles...
+ return type;
+ }
+
+ // unhandled is unknown
+ return null;
Review comment:
>As you said the code is volatile so chances of this path being hit are
even higher.
I mean, it's totally not possible to hit this null because there isn't a
`ExprType` combination of input arguments that ends up on this line. By
volatile I meant that this enum and file is likely going to go away and maybe
this function migrated into `ValueType`. This line is essentially a placeholder
for `ValueType.COMPLEX` which doesn't exist in `ExprType`, so I was shaping
this method to be straightforward to work off of that enum instead of this one
someday.
I can throw an exception with the messaging 'impossible' if you would
prefer, but there isn't a way to actually check it with a test.
##########
File path: core/src/main/java/org/apache/druid/math/expr/ConstantExpr.java
##########
@@ -35,6 +35,19 @@
*/
abstract class ConstantExpr implements Expr
{
+ final ExprType outputType;
+
+ protected ConstantExpr(ExprType outputType)
+ {
+ this.outputType = outputType;
+ }
+
+ @Override
Review comment:
Oops, I didn't actually mean to make this not be `@Nullable`, though I
guess it is true, but will fix to just be consistent with the interface.
##########
File path: core/src/main/java/org/apache/druid/math/expr/Expr.java
##########
@@ -116,16 +116,39 @@ default String getBindingIfIdentifier()
void visit(Visitor visitor);
/**
- * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.Each
{@link Expr} is responsible for
+ * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.
Each {@link Expr} is responsible for
* ensuring the {@link Shuttle} can visit all of its {@link Expr} children,
as well as updating its children
* {@link Expr} with the results from the {@link Shuttle}, before finally
visiting an updated form of itself.
*/
Expr visit(Shuttle shuttle);
/**
- * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingDetails}
+ * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingAnalysis}
*/
- BindingDetails analyzeInputs();
+ BindingAnalysis analyzeInputs();
+
+ /**
+ * Given an {@link InputBindingTypes}, compute what the output {@link
ExprType} will be for this expression. A return
+ * value of null indicates that the given type information was not enough to
resolve the output type, so the
+ * expression must be evaluated using default {@link #eval} handling where
types are only known after evaluation,
+ * through {@link ExprEval#type}.
+ */
+ @Nullable
+ default ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return null;
+ }
+
+ /**
+ * Mechanism to supply input types for the bindings which will back {@link
IdentifierExpr}, to use in the aid of
+ * inferring the output type of an expression with {@link #getOutputType}. A
null value means that either the binding
Review comment:
not long term probably, but currently this will be backed by
`ColumnCapabilities` from a `ColumnInspector`, which does not currently
distinguish between those 2 things for all implementations.
##########
File path: core/src/main/java/org/apache/druid/math/expr/UnaryOperatorExpr.java
##########
@@ -163,4 +171,15 @@ public String toString()
{
return StringUtils.format("!%s", expr);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
Review comment:
I don't know off the top of my head why it does that, I was just trying
to model its output. The binary logic expressions do that too:
```
@Override
protected ExprEval evalString(@Nullable String left, @Nullable String
right)
{
return
ExprEval.ofLongBoolean(Comparators.<String>naturalNullsFirst().compare(left,
right) < 0);
}
```
so it is just being consistent with those operators I think.
I'm moderately in the camp of wondering why these logical operators return
anything other than long for any input type, but I haven't thought fully
through on the implications of changing that yet, so maybe there is a reason
that the type is preserved for doubles in these operators.
##########
File path:
processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java
##########
@@ -93,6 +95,13 @@ public Expr visit(Shuttle shuttle)
return shuttle.visit(new TimestampCeilExpr(newArgs));
}
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return ExprType.LONG;
Review comment:
Hmm, so numeric null handling should be done currently be done using
`ExprEval.isNumericNull`, which will return true even if the type is a null
string from a `StringExprEval`. So i think 'string' is technically correct, but
I don't think spiritually or functionally quite true, nor that useful. We
probably should modify these `ExprEval.of(null)` to make type correct
`ExprEval` to make `eval` match exactly what `getOutputType` says, but I don't
know if this PR is the correct place to do that.
##########
File path:
server/src/main/java/org/apache/druid/server/lookup/cache/LookupCoordinatorManager.java
##########
@@ -518,7 +518,7 @@ private void initializeLookupsConfigWatcher()
configManager.set(
LOOKUP_CONFIG_KEY,
converted,
- new AuditInfo("autoConversion", "autoConversion", "127.0.0.1")
+ new AuditInfo("autoTypeConversion", "autoTypeConversion",
"127.0.0.1")
Review comment:
oops, intellij got a bit aggro
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprType.java
##########
@@ -29,5 +35,116 @@
STRING,
DOUBLE_ARRAY,
LONG_ARRAY,
- STRING_ARRAY
+ STRING_ARRAY;
+
+ public boolean isNumeric()
+ {
+ return isNumeric(this);
+ }
+
+ /**
+ * The expression system does not distinguish between {@link
ValueType#FLOAT} and {@link ValueType#DOUBLE}, and
+ * cannot currently handle {@link ValueType#COMPLEX} inputs. This method
will convert {@link ValueType#FLOAT} to
+ * {@link #DOUBLE}, or throw an exception if a {@link ValueType#COMPLEX} is
encountered.
+ *
+ * @throws IllegalStateException
+ */
+ public static ExprType fromValueType(@Nullable ValueType valueType)
+ {
+ if (valueType == null) {
+ throw new IllegalStateException("Unsupported unknown value type");
+ }
+ switch (valueType) {
+ case LONG:
+ return LONG;
+ case LONG_ARRAY:
+ return LONG_ARRAY;
+ case FLOAT:
+ case DOUBLE:
+ return DOUBLE;
+ case DOUBLE_ARRAY:
+ return DOUBLE_ARRAY;
+ case STRING:
+ return STRING;
+ case STRING_ARRAY:
+ return STRING_ARRAY;
+ case COMPLEX:
+ default:
+ throw new ISE("Unsupported value type[%s]", valueType);
+ }
+ }
+
+ public static boolean isNumeric(ExprType type)
+ {
+ return LONG.equals(type) || DOUBLE.equals(type);
+ }
+
+ public static boolean isArray(@Nullable ExprType type)
+ {
+ return LONG_ARRAY.equals(type) || DOUBLE_ARRAY.equals(type) ||
STRING_ARRAY.equals(type);
+ }
+
+ @Nullable
+ public static ExprType elementType(@Nullable ExprType type)
+ {
+ if (type != null && isArray(type)) {
+ switch (type) {
+ case STRING_ARRAY:
+ return STRING;
+ case LONG_ARRAY:
+ return LONG;
+ case DOUBLE_ARRAY:
+ return DOUBLE;
+ }
+ }
+ return type;
+ }
+
+ @Nullable
+ public static ExprType asArrayType(@Nullable ExprType elementType)
+ {
+ if (elementType != null && !isArray(elementType)) {
+ switch (elementType) {
+ case STRING:
+ return STRING_ARRAY;
+ case LONG:
+ return LONG_ARRAY;
+ case DOUBLE:
+ return DOUBLE_ARRAY;
+ }
+ }
+ return elementType;
+ }
+
+ @Nullable
+ public static ExprType implicitCast(@Nullable ExprType type, @Nullable
ExprType other)
+ {
+ if (type == null || other == null) {
+ // cannot implicitly cast unknown types
+ return null;
+ }
+ // arrays cannot be implicitly cast
+ if (isArray(type)) {
+ 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;
+ }
+
+ if (isNumeric(type) && isNumeric(other)) {
+ // all numbers win over longs
+ if (LONG.equals(type)) {
+ return other;
+ }
+ // floats vs longs would be handled here, but we currently only support
doubles...
+ return type;
+ }
+
+ // unhandled is unknown
+ return null;
Review comment:
>As you said the code is volatile so chances of this path being hit are
even higher.
I mean, it's totally not possible to hit this null because there isn't a
`ExprType` combination of input arguments that ends up on this line. By
volatile I meant that this enum and file is likely going to go away and maybe
this function migrated into `ValueType`. This line is essentially a placeholder
for `ValueType.COMPLEX` which doesn't exist in `ExprType`, so I was shaping
this method to be straightforward to work off of that enum instead of this one
someday.
I can throw an exception with the messaging 'impossible' if you would
prefer, but there isn't a way to actually check it with a test.
##########
File path: core/src/main/java/org/apache/druid/math/expr/ConstantExpr.java
##########
@@ -35,6 +35,19 @@
*/
abstract class ConstantExpr implements Expr
{
+ final ExprType outputType;
+
+ protected ConstantExpr(ExprType outputType)
+ {
+ this.outputType = outputType;
+ }
+
+ @Override
Review comment:
Oops, I didn't actually mean to make this not be `@Nullable`, though I
guess it is true, but will fix to just be consistent with the interface.
##########
File path: core/src/main/java/org/apache/druid/math/expr/Expr.java
##########
@@ -116,16 +116,39 @@ default String getBindingIfIdentifier()
void visit(Visitor visitor);
/**
- * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.Each
{@link Expr} is responsible for
+ * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.
Each {@link Expr} is responsible for
* ensuring the {@link Shuttle} can visit all of its {@link Expr} children,
as well as updating its children
* {@link Expr} with the results from the {@link Shuttle}, before finally
visiting an updated form of itself.
*/
Expr visit(Shuttle shuttle);
/**
- * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingDetails}
+ * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingAnalysis}
*/
- BindingDetails analyzeInputs();
+ BindingAnalysis analyzeInputs();
+
+ /**
+ * Given an {@link InputBindingTypes}, compute what the output {@link
ExprType} will be for this expression. A return
+ * value of null indicates that the given type information was not enough to
resolve the output type, so the
+ * expression must be evaluated using default {@link #eval} handling where
types are only known after evaluation,
+ * through {@link ExprEval#type}.
+ */
+ @Nullable
+ default ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return null;
+ }
+
+ /**
+ * Mechanism to supply input types for the bindings which will back {@link
IdentifierExpr}, to use in the aid of
+ * inferring the output type of an expression with {@link #getOutputType}. A
null value means that either the binding
Review comment:
not long term probably, but currently this will be backed by
`ColumnCapabilities` from a `ColumnInspector`, which does not currently
distinguish between those 2 things for all implementations.
##########
File path: core/src/main/java/org/apache/druid/math/expr/UnaryOperatorExpr.java
##########
@@ -163,4 +171,15 @@ public String toString()
{
return StringUtils.format("!%s", expr);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
Review comment:
I don't know off the top of my head why it does that, I was just trying
to model its output. The binary logic expressions do that too:
```
@Override
protected ExprEval evalString(@Nullable String left, @Nullable String
right)
{
return
ExprEval.ofLongBoolean(Comparators.<String>naturalNullsFirst().compare(left,
right) < 0);
}
```
so it is just being consistent with those operators I think.
I'm moderately in the camp of wondering why these logical operators return
anything other than long for any input type, but I haven't thought fully
through on the implications of changing that yet, so maybe there is a reason
that the type is preserved for doubles in these operators.
##########
File path:
processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java
##########
@@ -93,6 +95,13 @@ public Expr visit(Shuttle shuttle)
return shuttle.visit(new TimestampCeilExpr(newArgs));
}
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return ExprType.LONG;
Review comment:
Hmm, so numeric null handling should be done currently be done using
`ExprEval.isNumericNull`, which will return true even if the type is a null
string from a `StringExprEval`. So i think 'string' is technically correct, but
I don't think spiritually or functionally quite true, nor that useful. We
probably should modify these `ExprEval.of(null)` to make type correct
`ExprEval` to make `eval` match exactly what `getOutputType` says, but I don't
know if this PR is the correct place to do that.
##########
File path:
server/src/main/java/org/apache/druid/server/lookup/cache/LookupCoordinatorManager.java
##########
@@ -518,7 +518,7 @@ private void initializeLookupsConfigWatcher()
configManager.set(
LOOKUP_CONFIG_KEY,
converted,
- new AuditInfo("autoConversion", "autoConversion", "127.0.0.1")
+ new AuditInfo("autoTypeConversion", "autoTypeConversion",
"127.0.0.1")
Review comment:
oops, intellij got a bit aggro
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprType.java
##########
@@ -29,5 +35,116 @@
STRING,
DOUBLE_ARRAY,
LONG_ARRAY,
- STRING_ARRAY
+ STRING_ARRAY;
+
+ public boolean isNumeric()
+ {
+ return isNumeric(this);
+ }
+
+ /**
+ * The expression system does not distinguish between {@link
ValueType#FLOAT} and {@link ValueType#DOUBLE}, and
+ * cannot currently handle {@link ValueType#COMPLEX} inputs. This method
will convert {@link ValueType#FLOAT} to
+ * {@link #DOUBLE}, or throw an exception if a {@link ValueType#COMPLEX} is
encountered.
+ *
+ * @throws IllegalStateException
+ */
+ public static ExprType fromValueType(@Nullable ValueType valueType)
+ {
+ if (valueType == null) {
+ throw new IllegalStateException("Unsupported unknown value type");
+ }
+ switch (valueType) {
+ case LONG:
+ return LONG;
+ case LONG_ARRAY:
+ return LONG_ARRAY;
+ case FLOAT:
+ case DOUBLE:
+ return DOUBLE;
+ case DOUBLE_ARRAY:
+ return DOUBLE_ARRAY;
+ case STRING:
+ return STRING;
+ case STRING_ARRAY:
+ return STRING_ARRAY;
+ case COMPLEX:
+ default:
+ throw new ISE("Unsupported value type[%s]", valueType);
+ }
+ }
+
+ public static boolean isNumeric(ExprType type)
+ {
+ return LONG.equals(type) || DOUBLE.equals(type);
+ }
+
+ public static boolean isArray(@Nullable ExprType type)
+ {
+ return LONG_ARRAY.equals(type) || DOUBLE_ARRAY.equals(type) ||
STRING_ARRAY.equals(type);
+ }
+
+ @Nullable
+ public static ExprType elementType(@Nullable ExprType type)
+ {
+ if (type != null && isArray(type)) {
+ switch (type) {
+ case STRING_ARRAY:
+ return STRING;
+ case LONG_ARRAY:
+ return LONG;
+ case DOUBLE_ARRAY:
+ return DOUBLE;
+ }
+ }
+ return type;
+ }
+
+ @Nullable
+ public static ExprType asArrayType(@Nullable ExprType elementType)
+ {
+ if (elementType != null && !isArray(elementType)) {
+ switch (elementType) {
+ case STRING:
+ return STRING_ARRAY;
+ case LONG:
+ return LONG_ARRAY;
+ case DOUBLE:
+ return DOUBLE_ARRAY;
+ }
+ }
+ return elementType;
+ }
+
+ @Nullable
+ public static ExprType implicitCast(@Nullable ExprType type, @Nullable
ExprType other)
+ {
+ if (type == null || other == null) {
+ // cannot implicitly cast unknown types
+ return null;
+ }
+ // arrays cannot be implicitly cast
+ if (isArray(type)) {
+ 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;
+ }
+
+ if (isNumeric(type) && isNumeric(other)) {
+ // all numbers win over longs
+ if (LONG.equals(type)) {
+ return other;
+ }
+ // floats vs longs would be handled here, but we currently only support
doubles...
+ return type;
+ }
+
+ // unhandled is unknown
+ return null;
Review comment:
>As you said the code is volatile so chances of this path being hit are
even higher.
I mean, it's totally not possible to hit this null because there isn't a
`ExprType` combination of input arguments that ends up on this line. By
volatile I meant that this enum and file is likely going to go away and maybe
this function migrated into `ValueType`. This line is essentially a placeholder
for `ValueType.COMPLEX` which doesn't exist in `ExprType`, so I was shaping
this method to be straightforward to work off of that enum instead of this one
someday.
I can throw an exception with the messaging 'impossible' if you would
prefer, but there isn't a way to actually check it with a test.
##########
File path: core/src/main/java/org/apache/druid/math/expr/ConstantExpr.java
##########
@@ -35,6 +35,19 @@
*/
abstract class ConstantExpr implements Expr
{
+ final ExprType outputType;
+
+ protected ConstantExpr(ExprType outputType)
+ {
+ this.outputType = outputType;
+ }
+
+ @Override
Review comment:
Oops, I didn't actually mean to make this not be `@Nullable`, though I
guess it is true, but will fix to just be consistent with the interface.
##########
File path: core/src/main/java/org/apache/druid/math/expr/Expr.java
##########
@@ -116,16 +116,39 @@ default String getBindingIfIdentifier()
void visit(Visitor visitor);
/**
- * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.Each
{@link Expr} is responsible for
+ * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.
Each {@link Expr} is responsible for
* ensuring the {@link Shuttle} can visit all of its {@link Expr} children,
as well as updating its children
* {@link Expr} with the results from the {@link Shuttle}, before finally
visiting an updated form of itself.
*/
Expr visit(Shuttle shuttle);
/**
- * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingDetails}
+ * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingAnalysis}
*/
- BindingDetails analyzeInputs();
+ BindingAnalysis analyzeInputs();
+
+ /**
+ * Given an {@link InputBindingTypes}, compute what the output {@link
ExprType} will be for this expression. A return
+ * value of null indicates that the given type information was not enough to
resolve the output type, so the
+ * expression must be evaluated using default {@link #eval} handling where
types are only known after evaluation,
+ * through {@link ExprEval#type}.
+ */
+ @Nullable
+ default ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return null;
+ }
+
+ /**
+ * Mechanism to supply input types for the bindings which will back {@link
IdentifierExpr}, to use in the aid of
+ * inferring the output type of an expression with {@link #getOutputType}. A
null value means that either the binding
Review comment:
not long term probably, but currently this will be backed by
`ColumnCapabilities` from a `ColumnInspector`, which does not currently
distinguish between those 2 things for all implementations.
##########
File path: core/src/main/java/org/apache/druid/math/expr/UnaryOperatorExpr.java
##########
@@ -163,4 +171,15 @@ public String toString()
{
return StringUtils.format("!%s", expr);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
Review comment:
I don't know off the top of my head why it does that, I was just trying
to model its output. The binary logic expressions do that too:
```
@Override
protected ExprEval evalString(@Nullable String left, @Nullable String
right)
{
return
ExprEval.ofLongBoolean(Comparators.<String>naturalNullsFirst().compare(left,
right) < 0);
}
```
so it is just being consistent with those operators I think.
I'm moderately in the camp of wondering why these logical operators return
anything other than long for any input type, but I haven't thought fully
through on the implications of changing that yet, so maybe there is a reason
that the type is preserved for doubles in these operators.
##########
File path:
processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java
##########
@@ -93,6 +95,13 @@ public Expr visit(Shuttle shuttle)
return shuttle.visit(new TimestampCeilExpr(newArgs));
}
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return ExprType.LONG;
Review comment:
Hmm, so numeric null handling should be done currently be done using
`ExprEval.isNumericNull`, which will return true even if the type is a null
string from a `StringExprEval`. So i think 'string' is technically correct, but
I don't think spiritually or functionally quite true, nor that useful. We
probably should modify these `ExprEval.of(null)` to make type correct
`ExprEval` to make `eval` match exactly what `getOutputType` says, but I don't
know if this PR is the correct place to do that.
##########
File path:
server/src/main/java/org/apache/druid/server/lookup/cache/LookupCoordinatorManager.java
##########
@@ -518,7 +518,7 @@ private void initializeLookupsConfigWatcher()
configManager.set(
LOOKUP_CONFIG_KEY,
converted,
- new AuditInfo("autoConversion", "autoConversion", "127.0.0.1")
+ new AuditInfo("autoTypeConversion", "autoTypeConversion",
"127.0.0.1")
Review comment:
oops, intellij got a bit aggro
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprType.java
##########
@@ -29,5 +35,116 @@
STRING,
DOUBLE_ARRAY,
LONG_ARRAY,
- STRING_ARRAY
+ STRING_ARRAY;
+
+ public boolean isNumeric()
+ {
+ return isNumeric(this);
+ }
+
+ /**
+ * The expression system does not distinguish between {@link
ValueType#FLOAT} and {@link ValueType#DOUBLE}, and
+ * cannot currently handle {@link ValueType#COMPLEX} inputs. This method
will convert {@link ValueType#FLOAT} to
+ * {@link #DOUBLE}, or throw an exception if a {@link ValueType#COMPLEX} is
encountered.
+ *
+ * @throws IllegalStateException
+ */
+ public static ExprType fromValueType(@Nullable ValueType valueType)
+ {
+ if (valueType == null) {
+ throw new IllegalStateException("Unsupported unknown value type");
+ }
+ switch (valueType) {
+ case LONG:
+ return LONG;
+ case LONG_ARRAY:
+ return LONG_ARRAY;
+ case FLOAT:
+ case DOUBLE:
+ return DOUBLE;
+ case DOUBLE_ARRAY:
+ return DOUBLE_ARRAY;
+ case STRING:
+ return STRING;
+ case STRING_ARRAY:
+ return STRING_ARRAY;
+ case COMPLEX:
+ default:
+ throw new ISE("Unsupported value type[%s]", valueType);
+ }
+ }
+
+ public static boolean isNumeric(ExprType type)
+ {
+ return LONG.equals(type) || DOUBLE.equals(type);
+ }
+
+ public static boolean isArray(@Nullable ExprType type)
+ {
+ return LONG_ARRAY.equals(type) || DOUBLE_ARRAY.equals(type) ||
STRING_ARRAY.equals(type);
+ }
+
+ @Nullable
+ public static ExprType elementType(@Nullable ExprType type)
+ {
+ if (type != null && isArray(type)) {
+ switch (type) {
+ case STRING_ARRAY:
+ return STRING;
+ case LONG_ARRAY:
+ return LONG;
+ case DOUBLE_ARRAY:
+ return DOUBLE;
+ }
+ }
+ return type;
+ }
+
+ @Nullable
+ public static ExprType asArrayType(@Nullable ExprType elementType)
+ {
+ if (elementType != null && !isArray(elementType)) {
+ switch (elementType) {
+ case STRING:
+ return STRING_ARRAY;
+ case LONG:
+ return LONG_ARRAY;
+ case DOUBLE:
+ return DOUBLE_ARRAY;
+ }
+ }
+ return elementType;
+ }
+
+ @Nullable
+ public static ExprType implicitCast(@Nullable ExprType type, @Nullable
ExprType other)
+ {
+ if (type == null || other == null) {
+ // cannot implicitly cast unknown types
+ return null;
+ }
+ // arrays cannot be implicitly cast
+ if (isArray(type)) {
+ 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;
+ }
+
+ if (isNumeric(type) && isNumeric(other)) {
+ // all numbers win over longs
+ if (LONG.equals(type)) {
+ return other;
+ }
+ // floats vs longs would be handled here, but we currently only support
doubles...
+ return type;
+ }
+
+ // unhandled is unknown
+ return null;
Review comment:
>As you said the code is volatile so chances of this path being hit are
even higher.
I mean, it's totally not possible to hit this null because there isn't a
`ExprType` combination of input arguments that ends up on this line. By
volatile I meant that this enum and file is likely going to go away and maybe
this function migrated into `ValueType`. This line is essentially a placeholder
for `ValueType.COMPLEX` which doesn't exist in `ExprType`, so I was shaping
this method to be straightforward to work off of that enum instead of this one
someday.
I can throw an exception with the messaging 'impossible' if you would
prefer, but there isn't a way to actually check it with a test.
##########
File path: core/src/main/java/org/apache/druid/math/expr/ConstantExpr.java
##########
@@ -35,6 +35,19 @@
*/
abstract class ConstantExpr implements Expr
{
+ final ExprType outputType;
+
+ protected ConstantExpr(ExprType outputType)
+ {
+ this.outputType = outputType;
+ }
+
+ @Override
Review comment:
Oops, I didn't actually mean to make this not be `@Nullable`, though I
guess it is true, but will fix to just be consistent with the interface.
##########
File path: core/src/main/java/org/apache/druid/math/expr/Expr.java
##########
@@ -116,16 +116,39 @@ default String getBindingIfIdentifier()
void visit(Visitor visitor);
/**
- * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.Each
{@link Expr} is responsible for
+ * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.
Each {@link Expr} is responsible for
* ensuring the {@link Shuttle} can visit all of its {@link Expr} children,
as well as updating its children
* {@link Expr} with the results from the {@link Shuttle}, before finally
visiting an updated form of itself.
*/
Expr visit(Shuttle shuttle);
/**
- * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingDetails}
+ * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingAnalysis}
*/
- BindingDetails analyzeInputs();
+ BindingAnalysis analyzeInputs();
+
+ /**
+ * Given an {@link InputBindingTypes}, compute what the output {@link
ExprType} will be for this expression. A return
+ * value of null indicates that the given type information was not enough to
resolve the output type, so the
+ * expression must be evaluated using default {@link #eval} handling where
types are only known after evaluation,
+ * through {@link ExprEval#type}.
+ */
+ @Nullable
+ default ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return null;
+ }
+
+ /**
+ * Mechanism to supply input types for the bindings which will back {@link
IdentifierExpr}, to use in the aid of
+ * inferring the output type of an expression with {@link #getOutputType}. A
null value means that either the binding
Review comment:
not long term probably, but currently this will be backed by
`ColumnCapabilities` from a `ColumnInspector`, which does not currently
distinguish between those 2 things for all implementations.
##########
File path: core/src/main/java/org/apache/druid/math/expr/UnaryOperatorExpr.java
##########
@@ -163,4 +171,15 @@ public String toString()
{
return StringUtils.format("!%s", expr);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
Review comment:
I don't know off the top of my head why it does that, I was just trying
to model its output. The binary logic expressions do that too:
```
@Override
protected ExprEval evalString(@Nullable String left, @Nullable String
right)
{
return
ExprEval.ofLongBoolean(Comparators.<String>naturalNullsFirst().compare(left,
right) < 0);
}
```
so it is just being consistent with those operators I think.
I'm moderately in the camp of wondering why these logical operators return
anything other than long for any input type, but I haven't thought fully
through on the implications of changing that yet, so maybe there is a reason
that the type is preserved for doubles in these operators.
##########
File path:
processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java
##########
@@ -93,6 +95,13 @@ public Expr visit(Shuttle shuttle)
return shuttle.visit(new TimestampCeilExpr(newArgs));
}
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return ExprType.LONG;
Review comment:
Hmm, so numeric null handling should be done currently be done using
`ExprEval.isNumericNull`, which will return true even if the type is a null
string from a `StringExprEval`. So i think 'string' is technically correct, but
I don't think spiritually or functionally quite true, nor that useful. We
probably should modify these `ExprEval.of(null)` to make type correct
`ExprEval` to make `eval` match exactly what `getOutputType` says, but I don't
know if this PR is the correct place to do that.
##########
File path:
server/src/main/java/org/apache/druid/server/lookup/cache/LookupCoordinatorManager.java
##########
@@ -518,7 +518,7 @@ private void initializeLookupsConfigWatcher()
configManager.set(
LOOKUP_CONFIG_KEY,
converted,
- new AuditInfo("autoConversion", "autoConversion", "127.0.0.1")
+ new AuditInfo("autoTypeConversion", "autoTypeConversion",
"127.0.0.1")
Review comment:
oops, intellij got a bit aggro
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprType.java
##########
@@ -29,5 +35,116 @@
STRING,
DOUBLE_ARRAY,
LONG_ARRAY,
- STRING_ARRAY
+ STRING_ARRAY;
+
+ public boolean isNumeric()
+ {
+ return isNumeric(this);
+ }
+
+ /**
+ * The expression system does not distinguish between {@link
ValueType#FLOAT} and {@link ValueType#DOUBLE}, and
+ * cannot currently handle {@link ValueType#COMPLEX} inputs. This method
will convert {@link ValueType#FLOAT} to
+ * {@link #DOUBLE}, or throw an exception if a {@link ValueType#COMPLEX} is
encountered.
+ *
+ * @throws IllegalStateException
+ */
+ public static ExprType fromValueType(@Nullable ValueType valueType)
+ {
+ if (valueType == null) {
+ throw new IllegalStateException("Unsupported unknown value type");
+ }
+ switch (valueType) {
+ case LONG:
+ return LONG;
+ case LONG_ARRAY:
+ return LONG_ARRAY;
+ case FLOAT:
+ case DOUBLE:
+ return DOUBLE;
+ case DOUBLE_ARRAY:
+ return DOUBLE_ARRAY;
+ case STRING:
+ return STRING;
+ case STRING_ARRAY:
+ return STRING_ARRAY;
+ case COMPLEX:
+ default:
+ throw new ISE("Unsupported value type[%s]", valueType);
+ }
+ }
+
+ public static boolean isNumeric(ExprType type)
+ {
+ return LONG.equals(type) || DOUBLE.equals(type);
+ }
+
+ public static boolean isArray(@Nullable ExprType type)
+ {
+ return LONG_ARRAY.equals(type) || DOUBLE_ARRAY.equals(type) ||
STRING_ARRAY.equals(type);
+ }
+
+ @Nullable
+ public static ExprType elementType(@Nullable ExprType type)
+ {
+ if (type != null && isArray(type)) {
+ switch (type) {
+ case STRING_ARRAY:
+ return STRING;
+ case LONG_ARRAY:
+ return LONG;
+ case DOUBLE_ARRAY:
+ return DOUBLE;
+ }
+ }
+ return type;
+ }
+
+ @Nullable
+ public static ExprType asArrayType(@Nullable ExprType elementType)
+ {
+ if (elementType != null && !isArray(elementType)) {
+ switch (elementType) {
+ case STRING:
+ return STRING_ARRAY;
+ case LONG:
+ return LONG_ARRAY;
+ case DOUBLE:
+ return DOUBLE_ARRAY;
+ }
+ }
+ return elementType;
+ }
+
+ @Nullable
+ public static ExprType implicitCast(@Nullable ExprType type, @Nullable
ExprType other)
+ {
+ if (type == null || other == null) {
+ // cannot implicitly cast unknown types
+ return null;
+ }
+ // arrays cannot be implicitly cast
+ if (isArray(type)) {
+ 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;
+ }
+
+ if (isNumeric(type) && isNumeric(other)) {
+ // all numbers win over longs
+ if (LONG.equals(type)) {
+ return other;
+ }
+ // floats vs longs would be handled here, but we currently only support
doubles...
+ return type;
+ }
+
+ // unhandled is unknown
+ return null;
Review comment:
>As you said the code is volatile so chances of this path being hit are
even higher.
I mean, it's totally not possible to hit this null because there isn't a
`ExprType` combination of input arguments that ends up on this line. By
volatile I meant that this enum and file is likely going to go away and maybe
this function migrated into `ValueType`. This line is essentially a placeholder
for `ValueType.COMPLEX` which doesn't exist in `ExprType`, so I was shaping
this method to be straightforward to work off of that enum instead of this one
someday.
I can throw an exception with the messaging 'impossible' if you would
prefer, but there isn't a way to actually check it with a test.
##########
File path: core/src/main/java/org/apache/druid/math/expr/ConstantExpr.java
##########
@@ -35,6 +35,19 @@
*/
abstract class ConstantExpr implements Expr
{
+ final ExprType outputType;
+
+ protected ConstantExpr(ExprType outputType)
+ {
+ this.outputType = outputType;
+ }
+
+ @Override
Review comment:
Oops, I didn't actually mean to make this not be `@Nullable`, though I
guess it is true, but will fix to just be consistent with the interface.
##########
File path: core/src/main/java/org/apache/druid/math/expr/Expr.java
##########
@@ -116,16 +116,39 @@ default String getBindingIfIdentifier()
void visit(Visitor visitor);
/**
- * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.Each
{@link Expr} is responsible for
+ * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.
Each {@link Expr} is responsible for
* ensuring the {@link Shuttle} can visit all of its {@link Expr} children,
as well as updating its children
* {@link Expr} with the results from the {@link Shuttle}, before finally
visiting an updated form of itself.
*/
Expr visit(Shuttle shuttle);
/**
- * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingDetails}
+ * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingAnalysis}
*/
- BindingDetails analyzeInputs();
+ BindingAnalysis analyzeInputs();
+
+ /**
+ * Given an {@link InputBindingTypes}, compute what the output {@link
ExprType} will be for this expression. A return
+ * value of null indicates that the given type information was not enough to
resolve the output type, so the
+ * expression must be evaluated using default {@link #eval} handling where
types are only known after evaluation,
+ * through {@link ExprEval#type}.
+ */
+ @Nullable
+ default ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return null;
+ }
+
+ /**
+ * Mechanism to supply input types for the bindings which will back {@link
IdentifierExpr}, to use in the aid of
+ * inferring the output type of an expression with {@link #getOutputType}. A
null value means that either the binding
Review comment:
not long term probably, but currently this will be backed by
`ColumnCapabilities` from a `ColumnInspector`, which does not currently
distinguish between those 2 things for all implementations.
##########
File path: core/src/main/java/org/apache/druid/math/expr/UnaryOperatorExpr.java
##########
@@ -163,4 +171,15 @@ public String toString()
{
return StringUtils.format("!%s", expr);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
Review comment:
I don't know off the top of my head why it does that, I was just trying
to model its output. The binary logic expressions do that too:
```
@Override
protected ExprEval evalString(@Nullable String left, @Nullable String
right)
{
return
ExprEval.ofLongBoolean(Comparators.<String>naturalNullsFirst().compare(left,
right) < 0);
}
```
so it is just being consistent with those operators I think.
I'm moderately in the camp of wondering why these logical operators return
anything other than long for any input type, but I haven't thought fully
through on the implications of changing that yet, so maybe there is a reason
that the type is preserved for doubles in these operators.
##########
File path:
processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java
##########
@@ -93,6 +95,13 @@ public Expr visit(Shuttle shuttle)
return shuttle.visit(new TimestampCeilExpr(newArgs));
}
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return ExprType.LONG;
Review comment:
Hmm, so numeric null handling should be done currently be done using
`ExprEval.isNumericNull`, which will return true even if the type is a null
string from a `StringExprEval`. So i think 'string' is technically correct, but
I don't think spiritually or functionally quite true, nor that useful. We
probably should modify these `ExprEval.of(null)` to make type correct
`ExprEval` to make `eval` match exactly what `getOutputType` says, but I don't
know if this PR is the correct place to do that.
##########
File path:
server/src/main/java/org/apache/druid/server/lookup/cache/LookupCoordinatorManager.java
##########
@@ -518,7 +518,7 @@ private void initializeLookupsConfigWatcher()
configManager.set(
LOOKUP_CONFIG_KEY,
converted,
- new AuditInfo("autoConversion", "autoConversion", "127.0.0.1")
+ new AuditInfo("autoTypeConversion", "autoTypeConversion",
"127.0.0.1")
Review comment:
oops, intellij got a bit aggro
##########
File path: core/src/main/java/org/apache/druid/math/expr/ConstantExpr.java
##########
@@ -35,6 +35,19 @@
*/
abstract class ConstantExpr implements Expr
{
+ final ExprType outputType;
+
+ protected ConstantExpr(ExprType outputType)
+ {
+ this.outputType = outputType;
+ }
+
+ @Override
Review comment:
Oops, I didn't actually mean to make this not be `@Nullable`, though I
guess it is true, but will fix to just be consistent with the interface.
##########
File path: core/src/main/java/org/apache/druid/math/expr/Expr.java
##########
@@ -116,16 +116,39 @@ default String getBindingIfIdentifier()
void visit(Visitor visitor);
/**
- * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.Each
{@link Expr} is responsible for
+ * Programatically rewrite the {@link Expr} tree with a {@link Shuttle}.
Each {@link Expr} is responsible for
* ensuring the {@link Shuttle} can visit all of its {@link Expr} children,
as well as updating its children
* {@link Expr} with the results from the {@link Shuttle}, before finally
visiting an updated form of itself.
*/
Expr visit(Shuttle shuttle);
/**
- * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingDetails}
+ * Examine the usage of {@link IdentifierExpr} children of an {@link Expr},
constructing a {@link BindingAnalysis}
*/
- BindingDetails analyzeInputs();
+ BindingAnalysis analyzeInputs();
+
+ /**
+ * Given an {@link InputBindingTypes}, compute what the output {@link
ExprType} will be for this expression. A return
+ * value of null indicates that the given type information was not enough to
resolve the output type, so the
+ * expression must be evaluated using default {@link #eval} handling where
types are only known after evaluation,
+ * through {@link ExprEval#type}.
+ */
+ @Nullable
+ default ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return null;
+ }
+
+ /**
+ * Mechanism to supply input types for the bindings which will back {@link
IdentifierExpr}, to use in the aid of
+ * inferring the output type of an expression with {@link #getOutputType}. A
null value means that either the binding
Review comment:
not long term probably, but currently this will be backed by
`ColumnCapabilities` from a `ColumnInspector`, which does not currently
distinguish between those 2 things for all implementations.
##########
File path: core/src/main/java/org/apache/druid/math/expr/UnaryOperatorExpr.java
##########
@@ -163,4 +171,15 @@ public String toString()
{
return StringUtils.format("!%s", expr);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
Review comment:
I don't know off the top of my head why it does that, I was just trying
to model its output. The binary logic expressions do that too:
```
@Override
protected ExprEval evalString(@Nullable String left, @Nullable String
right)
{
return
ExprEval.ofLongBoolean(Comparators.<String>naturalNullsFirst().compare(left,
right) < 0);
}
```
so it is just being consistent with those operators I think.
I'm moderately in the camp of wondering why these logical operators return
anything other than long for any input type, but I haven't thought fully
through on the implications of changing that yet, so maybe there is a reason
that the type is preserved for doubles in these operators.
##########
File path:
processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java
##########
@@ -93,6 +95,13 @@ public Expr visit(Shuttle shuttle)
return shuttle.visit(new TimestampCeilExpr(newArgs));
}
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return ExprType.LONG;
Review comment:
Hmm, so numeric null handling should be done currently be done using
`ExprEval.isNumericNull`, which will return true even if the type is a null
string from a `StringExprEval`. So i think 'string' is technically correct, but
I don't think spiritually or functionally quite true, nor that useful. We
probably should modify these `ExprEval.of(null)` to make type correct
`ExprEval` to make `eval` match exactly what `getOutputType` says, but I don't
know if this PR is the correct place to do that.
##########
File path:
server/src/main/java/org/apache/druid/server/lookup/cache/LookupCoordinatorManager.java
##########
@@ -518,7 +518,7 @@ private void initializeLookupsConfigWatcher()
configManager.set(
LOOKUP_CONFIG_KEY,
converted,
- new AuditInfo("autoConversion", "autoConversion", "127.0.0.1")
+ new AuditInfo("autoTypeConversion", "autoTypeConversion",
"127.0.0.1")
Review comment:
oops, intellij got a bit aggro
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprType.java
##########
@@ -29,5 +35,116 @@
STRING,
DOUBLE_ARRAY,
LONG_ARRAY,
- STRING_ARRAY
+ STRING_ARRAY;
+
+ public boolean isNumeric()
+ {
+ return isNumeric(this);
+ }
+
+ /**
+ * The expression system does not distinguish between {@link
ValueType#FLOAT} and {@link ValueType#DOUBLE}, and
+ * cannot currently handle {@link ValueType#COMPLEX} inputs. This method
will convert {@link ValueType#FLOAT} to
+ * {@link #DOUBLE}, or throw an exception if a {@link ValueType#COMPLEX} is
encountered.
+ *
+ * @throws IllegalStateException
+ */
+ public static ExprType fromValueType(@Nullable ValueType valueType)
+ {
+ if (valueType == null) {
+ throw new IllegalStateException("Unsupported unknown value type");
+ }
+ switch (valueType) {
+ case LONG:
+ return LONG;
+ case LONG_ARRAY:
+ return LONG_ARRAY;
+ case FLOAT:
+ case DOUBLE:
+ return DOUBLE;
+ case DOUBLE_ARRAY:
+ return DOUBLE_ARRAY;
+ case STRING:
+ return STRING;
+ case STRING_ARRAY:
+ return STRING_ARRAY;
+ case COMPLEX:
+ default:
+ throw new ISE("Unsupported value type[%s]", valueType);
+ }
+ }
+
+ public static boolean isNumeric(ExprType type)
+ {
+ return LONG.equals(type) || DOUBLE.equals(type);
+ }
+
+ public static boolean isArray(@Nullable ExprType type)
+ {
+ return LONG_ARRAY.equals(type) || DOUBLE_ARRAY.equals(type) ||
STRING_ARRAY.equals(type);
+ }
+
+ @Nullable
+ public static ExprType elementType(@Nullable ExprType type)
+ {
+ if (type != null && isArray(type)) {
+ switch (type) {
+ case STRING_ARRAY:
+ return STRING;
+ case LONG_ARRAY:
+ return LONG;
+ case DOUBLE_ARRAY:
+ return DOUBLE;
+ }
+ }
+ return type;
+ }
+
+ @Nullable
+ public static ExprType asArrayType(@Nullable ExprType elementType)
+ {
+ if (elementType != null && !isArray(elementType)) {
+ switch (elementType) {
+ case STRING:
+ return STRING_ARRAY;
+ case LONG:
+ return LONG_ARRAY;
+ case DOUBLE:
+ return DOUBLE_ARRAY;
+ }
+ }
+ return elementType;
+ }
+
+ @Nullable
+ public static ExprType implicitCast(@Nullable ExprType type, @Nullable
ExprType other)
+ {
+ if (type == null || other == null) {
+ // cannot implicitly cast unknown types
+ return null;
+ }
+ // arrays cannot be implicitly cast
+ if (isArray(type)) {
+ 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;
+ }
+
+ if (isNumeric(type) && isNumeric(other)) {
+ // all numbers win over longs
+ if (LONG.equals(type)) {
+ return other;
+ }
+ // floats vs longs would be handled here, but we currently only support
doubles...
+ return type;
+ }
+
+ // unhandled is unknown
+ return null;
Review comment:
>As you said the code is volatile so chances of this path being hit are
even higher.
I mean, it's totally not possible to hit this null because there isn't a
`ExprType` combination of input arguments that ends up on this line. By
volatile I meant that this enum and file is likely going to go away and maybe
this function migrated into `ValueType`. This line is essentially a placeholder
for `ValueType.COMPLEX` which doesn't exist in `ExprType`, so I was shaping
this method to be straightforward to work off of that enum instead of this one
someday.
I can throw an exception with the messaging 'impossible' if you would
prefer, but there isn't a way to actually check it with a test.
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprType.java
##########
@@ -29,5 +35,116 @@
STRING,
DOUBLE_ARRAY,
LONG_ARRAY,
- STRING_ARRAY
+ STRING_ARRAY;
+
+ public boolean isNumeric()
+ {
+ return isNumeric(this);
+ }
+
+ /**
+ * The expression system does not distinguish between {@link
ValueType#FLOAT} and {@link ValueType#DOUBLE}, and
+ * cannot currently handle {@link ValueType#COMPLEX} inputs. This method
will convert {@link ValueType#FLOAT} to
+ * {@link #DOUBLE}, or throw an exception if a {@link ValueType#COMPLEX} is
encountered.
+ *
+ * @throws IllegalStateException
+ */
+ public static ExprType fromValueType(@Nullable ValueType valueType)
+ {
+ if (valueType == null) {
+ throw new IllegalStateException("Unsupported unknown value type");
+ }
+ switch (valueType) {
+ case LONG:
+ return LONG;
+ case LONG_ARRAY:
+ return LONG_ARRAY;
+ case FLOAT:
+ case DOUBLE:
+ return DOUBLE;
+ case DOUBLE_ARRAY:
+ return DOUBLE_ARRAY;
+ case STRING:
+ return STRING;
+ case STRING_ARRAY:
+ return STRING_ARRAY;
+ case COMPLEX:
+ default:
+ throw new ISE("Unsupported value type[%s]", valueType);
+ }
+ }
+
+ public static boolean isNumeric(ExprType type)
+ {
+ return LONG.equals(type) || DOUBLE.equals(type);
+ }
+
+ public static boolean isArray(@Nullable ExprType type)
+ {
+ return LONG_ARRAY.equals(type) || DOUBLE_ARRAY.equals(type) ||
STRING_ARRAY.equals(type);
+ }
+
+ @Nullable
+ public static ExprType elementType(@Nullable ExprType type)
+ {
+ if (type != null && isArray(type)) {
+ switch (type) {
+ case STRING_ARRAY:
+ return STRING;
+ case LONG_ARRAY:
+ return LONG;
+ case DOUBLE_ARRAY:
+ return DOUBLE;
+ }
+ }
+ return type;
+ }
+
+ @Nullable
+ public static ExprType asArrayType(@Nullable ExprType elementType)
+ {
+ if (elementType != null && !isArray(elementType)) {
+ switch (elementType) {
+ case STRING:
+ return STRING_ARRAY;
+ case LONG:
+ return LONG_ARRAY;
+ case DOUBLE:
+ return DOUBLE_ARRAY;
+ }
+ }
+ return elementType;
+ }
+
+ @Nullable
+ public static ExprType implicitCast(@Nullable ExprType type, @Nullable
ExprType other)
+ {
+ if (type == null || other == null) {
+ // cannot implicitly cast unknown types
+ return null;
+ }
+ // arrays cannot be implicitly cast
+ if (isArray(type)) {
+ 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;
+ }
+
+ if (isNumeric(type) && isNumeric(other)) {
+ // all numbers win over longs
+ if (LONG.equals(type)) {
+ return other;
+ }
+ // floats vs longs would be handled here, but we currently only support
doubles...
+ return type;
+ }
+
+ // unhandled is unknown
+ return null;
Review comment:
>As you said the code is volatile so chances of this path being hit are
even higher.
I mean, it's totally not possible to hit this null because there isn't a
`ExprType` combination of input arguments that ends up on this line. By
volatile I meant that this enum and file is likely going to go away and maybe
this function migrated into `ValueType`. This line is essentially a placeholder
for `ValueType.COMPLEX` which doesn't exist in `ExprType`, so I was shaping
this method to be straightforward to work off of that enum instead of this one
someday.
I can throw an exception with the messaging 'impossible' if you would
prefer, but there isn't a way to actually check it with a test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
