suneet-s commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r486739284
##########
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:
nit: Add `@NonNull` since the super class says this is `Nullable`, I'm
not actually sure which takes precedence in this case when the package is
annotated with `EverythingIsNonNullByDefault`
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprType.java
##########
@@ -29,5 +35,119 @@
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)) {
Review comment:
nit: I don't think the `isArray` check is needed
```suggestion
if (elementType != null) {
```
##########
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:
note to self: Do we want null to mean 2 things?
##########
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:
The eval function returns `ExprEval.of(null)` if the value is a numeric
null. It looks like in that case the output type should be ExprType.STRING
##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprType.java
##########
@@ -29,5 +35,119 @@
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;
+ }
+
+ /**
+ * Given 2 'input' types, choose the most appropriate combined type, if
possible
+ */
+ @Nullable
+ public static ExprType autoTypeConversion(@Nullable ExprType type, @Nullable
ExprType other)
Review comment:
I think we should add a unit test to make sure we cover all the
different branches for this. There looks like there's a lot of subtlety in the
ordering of the if conditions and It sounds like an important function that
many others will rely on working correctly. I think this function should have
100% branch coverage
I think all the static functions in this class should be unit tested.
##########
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:
I think this is an accidental change
##########
File path:
processing/src/main/java/org/apache/druid/query/expression/TimestampParseExprMacro.java
##########
@@ -100,6 +102,13 @@ public Expr visit(Shuttle shuttle)
return shuttle.visit(new TimestampParseExpr(newArg));
}
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return ExprType.LONG;
Review comment:
Similar problem with `ExprEval.of(null)`
##########
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:
Why does the not operator need to translate a string to a long?
##########
File path:
processing/src/test/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacroTest.java
##########
@@ -203,5 +205,12 @@ public Expr visit(Shuttle shuttle)
{
return null;
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ return null;
+ }
Review comment:
nit: You can delete this now that this is the default behavior.
----------------------------------------------------------------
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]
