You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/09/09 01:45:52 UTC
[GitHub] [druid] clintropolis opened a new pull request #10370: add computed Expr output types
clintropolis opened a new pull request #10370:
URL: https://github.com/apache/druid/pull/10370
### Description
This PR adds the ability to compute the output type for a given expression, given the types of the input bindings. It is worth mentioning that _nothing is currently using or taking advantage of this_, and that this PR is scaffolding for follow-up work which will introduce vectorized expressions (and could potentially be used to optimize non-vectorized expressions as well).
Anyway, the main changes in this PR have been added to `Expr`:
```java
@Nullable
ExprType getOutputType(InputBindingTypes inputTypes);
interface InputBindingTypes
{
@Nullable
ExprType getType(String name);
}
```
with similar methods added to `Function` and `ApplyFunction` so that the output types of functions can be computed.
I think there is a lot of room for further improvement, for example this could potentially be combined with the input analysis, or maybe be transitioned into a sort of `optimize` call that can potentially produce a new `Expr` which already knows its output type, allowing for a `ExprType getOutputType()` with no arguments. Some of the function abstract classes feel a bit messy as well, however, I leave all that as work for another day.
It also includes a minor refactor salvaged from #10355 for shorthand of producing long typed boolean values.
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
<hr>
##### Key changed/added classes in this PR
* `Expr`
* `ExprType`
* all implementations of `Expr`, `Function`, and `ApplyFunction` can now produce type information.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r487140081
##########
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:
my thinking is that returning null here would surface as a bug someplace else. but if we throw an exception here itself, its easier to debug. As you said the code is volatile so chances of this path being hit are even higher.
##########
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:
my thinking is that returning null here would surface as a bug someplace else. but if we throw an exception here itself, its easier to debug. As you said the code is volatile so chances of this path being hit are even higher.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #10370:
URL: https://github.com/apache/druid/pull/10370#issuecomment-690762614
>This PR will cause soft conflicts (which will probably not show up as merge conflicts) with any open PRs that are adding any new Expr, ExprMacro, Function, ApplyFunction, since they will need to include a getOutputType implementation after this PR is merged.
I changed my mind and modified this PR to take a less opinionated approach. `Expr.getOutputType` now has a default implementation that will return null for the output type, so this PR should be less disruptive.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r486052004
##########
File path: core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java
##########
@@ -848,4 +882,32 @@ public CartesianFoldLambdaBinding accumulateWithIndex(int index, Object acc)
return this;
}
}
+
+ class LambdaInputBindingTypes implements Expr.InputBindingTypes
+ {
+ private final Object2IntMap<String> lambdaIdentifiers;
+ private final Expr.InputBindingTypes inputTypes;
+ private final List<Expr> args;
+
+ public LambdaInputBindingTypes(Expr.InputBindingTypes inputTypes, LambdaExpr expr, List<Expr> args)
+ {
+ this.inputTypes = inputTypes;
+ this.args = args;
+ List<String> identifiers = expr.getIdentifiers();
+ this.lambdaIdentifiers = new Object2IntOpenHashMap<>(args.size());
+ for (int i = 0; i < args.size(); i++) {
+ lambdaIdentifiers.put(identifiers.get(i), i);
+ }
+ }
+
+ @Nullable
+ @Override
+ public ExprType getType(String name)
+ {
+ if (lambdaIdentifiers.containsKey(name)) {
+ return ExprType.elementType(args.get(lambdaIdentifiers.getInt(name)).getOutputType(inputTypes));
Review comment:
nit: Willl getType be called in a loop anywhere? It might be better to use `getOrDefault(..)` instead to avoid 2 hashcode computations here (for `containsKey` and `getInt`)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r486762673
##########
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
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r488431930
##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
##########
@@ -57,6 +57,17 @@ protected final double evalDouble(double left, double right)
// Use Double.compare for more consistent NaN handling.
return Evals.asDouble(Double.compare(left, right) < 0);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
+ return implicitCast;
Review comment:
Thanks @clintropolis
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r487150998
##########
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r488075026
##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
##########
@@ -57,6 +57,17 @@ protected final double evalDouble(double left, double right)
// Use Double.compare for more consistent NaN handling.
return Evals.asDouble(Double.compare(left, right) < 0);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
+ return implicitCast;
Review comment:
can you elaborate more on this? How will that look like?
> This PR is setting up for vectorized expressions, which are going to be strongly typed before processing, which I think makes this inconsistent/confusing/magical behavior not necessary.
While I understand the right behavior is debatable, it may still be best to keep the type in sync with eval even if it's not very intuitive. It may require that the logic of figuring out the type is not reusable as a function `ExprType.autoTypeConversion` and may have to be written differently for some operators. We can, later on, change the different functions, fixing both the `eval` and `getOutputType` together. But till then, they will be in sync.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r486048490
##########
File path: core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java
##########
@@ -74,6 +75,9 @@ default boolean hasArrayOutput(LambdaExpr lambdaExpr)
*/
void validateArguments(LambdaExpr lambdaExpr, List<Expr> args);
+ @Nullable
+ ExprType getOutputType(Expr.InputBindingTypes inputTypes, LambdaExpr expr, List<Expr> args);
Review comment:
javadocs please
##########
File path: core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java
##########
@@ -848,4 +882,32 @@ public CartesianFoldLambdaBinding accumulateWithIndex(int index, Object acc)
return this;
}
}
+
+ class LambdaInputBindingTypes implements Expr.InputBindingTypes
Review comment:
javadocs please
##########
File path: core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java
##########
@@ -848,4 +882,32 @@ public CartesianFoldLambdaBinding accumulateWithIndex(int index, Object acc)
return this;
}
}
+
+ class LambdaInputBindingTypes implements Expr.InputBindingTypes
+ {
+ private final Object2IntMap<String> lambdaIdentifiers;
+ private final Expr.InputBindingTypes inputTypes;
+ private final List<Expr> args;
+
+ public LambdaInputBindingTypes(Expr.InputBindingTypes inputTypes, LambdaExpr expr, List<Expr> args)
+ {
+ this.inputTypes = inputTypes;
+ this.args = args;
+ List<String> identifiers = expr.getIdentifiers();
+ this.lambdaIdentifiers = new Object2IntOpenHashMap<>(args.size());
+ for (int i = 0; i < args.size(); i++) {
+ lambdaIdentifiers.put(identifiers.get(i), i);
+ }
+ }
+
+ @Nullable
+ @Override
+ public ExprType getType(String name)
+ {
+ if (lambdaIdentifiers.containsKey(name)) {
+ return ExprType.elementType(args.get(lambdaIdentifiers.getInt(name)).getOutputType(inputTypes));
Review comment:
Willl getType be called in a loop anywhere? It might be better to use `getOrDefault(..)` instead to avoid 2 hashcode computations here (for `containsKey` and `getInt`)
##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
##########
@@ -57,6 +57,17 @@ protected final double evalDouble(double left, double right)
// Use Double.compare for more consistent NaN handling.
return Evals.asDouble(Double.compare(left, right) < 0);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
+ return implicitCast;
Review comment:
Looking at how this function works got me thinking about some stuff... Does this function need to be in sync with the behavior in `BinaryEvalOpExprBase#eval` (I think so 🤔) Since the `eval` method isn't implemented here, would it be better to implement it in `BinaryEvalOpExprBase`?
Can you explain how `getOutputType` would deal with default null handling mode.
Also, what does it mean to have an output type of null?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r487317611
##########
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:
Thanks for the explanation. I agree that we shouldn't try to address that in this PR
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r486634787
##########
File path: core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java
##########
@@ -848,4 +882,32 @@ public CartesianFoldLambdaBinding accumulateWithIndex(int index, Object acc)
return this;
}
}
+
+ class LambdaInputBindingTypes implements Expr.InputBindingTypes
+ {
+ private final Object2IntMap<String> lambdaIdentifiers;
+ private final Expr.InputBindingTypes inputTypes;
+ private final List<Expr> args;
+
+ public LambdaInputBindingTypes(Expr.InputBindingTypes inputTypes, LambdaExpr expr, List<Expr> args)
+ {
+ this.inputTypes = inputTypes;
+ this.args = args;
+ List<String> identifiers = expr.getIdentifiers();
+ this.lambdaIdentifiers = new Object2IntOpenHashMap<>(args.size());
+ for (int i = 0; i < args.size(); i++) {
+ lambdaIdentifiers.put(identifiers.get(i), i);
+ }
+ }
+
+ @Nullable
+ @Override
+ public ExprType getType(String name)
+ {
+ if (lambdaIdentifiers.containsKey(name)) {
+ return ExprType.elementType(args.get(lambdaIdentifiers.getInt(name)).getOutputType(inputTypes));
Review comment:
It shouldn't be called in any sort of hot loop. The eventual usage of this information is so that this happens during a sort of planning phase that is checking to see if it we can make a strongly typed and optimized expression evaluator to use instead of the default.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r486627512
##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
##########
@@ -57,6 +57,17 @@ protected final double evalDouble(double left, double right)
// Use Double.compare for more consistent NaN handling.
return Evals.asDouble(Double.compare(left, right) < 0);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
+ return implicitCast;
Review comment:
>Looking at how this function works got me thinking about some stuff... Does this function need to be in sync with the behavior in BinaryEvalOpExprBase#eval (I think so 🤔) Since the eval method isn't implemented here, would it be better to implement it in BinaryEvalOpExprBase?
Yeah, the behavior of `getOutputType` should always match the behavior of `eval`. In this case, it can't really be pushed down because the math operators also implement `BinaryEvalOpExprBase`, but do not handle string inputs as numerical outputs. We could put another type in between these logical operators and `BinaryEvalOpExprBase` that provides it though, I will consider doing that.
>Can you explain how getOutputType would deal with default null handling mode.
`getOutputType` should be fully independent of how `druid.generic.useDefaultValueForNull` is set since it does not capture (currently) whether or not nulls can happen.
>Also, what does it mean to have an output type of null?
An output type of null signifies that we couldn't compute what the output type is, most likely because input type information isn't available. When the input to the expressions are actual segments (`QueryableIndexStorageAdapter`) then type information should always be available, and a `null` signifies an input that doesn't exist, but other adapters (and other usages of `Expr` such as for transforms) might not always have complete type information.
I will add all of this stuff to the javadocs.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r487317611
##########
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:
Thanks for the explanation. I agree that we shouldn't try to address that in this PR
##########
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:
Thanks for the explanation. I agree that we shouldn't try to address that in this PR
##########
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:
Thanks for the explanation. I agree that we shouldn't try to address that in this PR
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r487841757
##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
##########
@@ -57,6 +57,17 @@ protected final double evalDouble(double left, double right)
// Use Double.compare for more consistent NaN handling.
return Evals.asDouble(Double.compare(left, right) < 0);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
+ return implicitCast;
Review comment:
Good eye, you spotted a thing I was purposefully ignoring because I think it is not really great behavior, or consistent with other math functions. The default behavior for those 2 input math operators is to consider the arguments as doubles as you've noticed, regardless of whether or not they are both doubles. I suspect it is implemented like this to allow null values inputs to still work as zeros in 'default' mode (`druid.generic.useDefaultValueForNull=true`), because the current expressions are not strongly typed so these nulls all end up as string values. See [this comment](https://github.com/apache/druid/pull/10370#discussion_r486762521) for a bit more explanation, and [this comment](https://github.com/apache/druid/pull/10084#discussion_r446486836) for some of the problems that this causes.
It is also a bit unintuitive behavior. A string column which contains numeric values will work in one of these math operators, so something like `2.0 + '3.1' = 5.1` works magically. But it will also potentially treat the string value as 0 if the string is not a number in default mode, so `2.0 + 'foo' = 2.0` (or null in sql compatible mode). The math functions (`min`, `max`, etc) instead treat either input as string as a 0/null output without evaluating the function.
I'm not actually sure what the best behavior here is, the math function behavior seems a bit more intuitive to me, where either input being a string produces a null output, so I chose to use that here. This PR is setting up for vectorized expressions, which are going to be strongly typed before processing, which I think makes this inconsistent/confusing/magical behavior not necessary. I was planning to raise a discussion about this part in a future PR since it isn't actually affecting anything yet, but I think it is good to start talking about it even though it isn't wired up to anything yet.
Also, I really think 'default' mode complicates the expression system quite a lot. It would be my preference for the expression system to always behave in an SQL compatible manner, and default mode only come into play on the boundaries for data coming in and going out, but I haven't fully thought through the implications of this and it requires some discussion I think, and might be a bit dramatic of a change.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r485423245
##########
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:
shouldn't it instead throw an exception?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r486756143
##########
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.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r487140081
##########
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:
my thinking is that returning null here would surface as a bug someplace else. but if we throw an exception here itself, its easier to debug. As you said the code is volatile so chances of this path being hit are even higher.
##########
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:
my thinking is that returning null here would surface as a bug someplace else. but if we throw an exception here itself, its easier to debug. As you said the code is volatile so chances of this path being hit are even higher.
##########
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:
my thinking is that returning null here would surface as a bug someplace else. but if we throw an exception here itself, its easier to debug. As you said the code is volatile so chances of this path being hit are even higher.
##########
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:
my thinking is that returning null here would surface as a bug someplace else. but if we throw an exception here itself, its easier to debug. As you said the code is volatile so chances of this path being hit are even higher.
##########
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:
my thinking is that returning null here would surface as a bug someplace else. but if we throw an exception here itself, its easier to debug. As you said the code is volatile so chances of this path being hit are even higher.
##########
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:
my thinking is that returning null here would surface as a bug someplace else. but if we throw an exception here itself, its easier to debug. As you said the code is volatile so chances of this path being hit are even higher.
##########
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:
my thinking is that returning null here would surface as a bug someplace else. but if we throw an exception here itself, its easier to debug. As you said the code is volatile so chances of this path being hit are even higher.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r488324915
##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
##########
@@ -57,6 +57,17 @@ protected final double evalDouble(double left, double right)
// Use Double.compare for more consistent NaN handling.
return Evals.asDouble(Double.compare(left, right) < 0);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
+ return implicitCast;
Review comment:
I'm going to merge this PR, but we can continue this discussion in the next one, so this stuff can actually be tied into things in a concrete manner. Looking at postgres, its math functions do, at least somewhat, allow for implicit conversion of sting numbers to numbers. So `2.0 + '3.1' = 5.1` and `cos('1')` work, but `'2.0' + '3.1'` does not. In druid currently the first would have the same behavior as postgres, but the 2nd would be null or 0 depending on the value of `druid.generic.useDefaultValueForNull`, while the 3rd expression would do string concatenation.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
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.
##########
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:
Thanks for the explanation. I agree that we shouldn't try to address that in this PR
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r486757634
##########
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.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
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.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r487140081
##########
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:
my thinking is that returning null here would surface as a bug someplace else. but if we throw an exception here itself, its easier to debug. As you said the code is volatile so chances of this path being hit are even higher.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
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.
##########
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:
Thanks for the explanation. I agree that we shouldn't try to address that in this PR
##########
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.
##########
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:
Thanks for the explanation. I agree that we shouldn't try to address that in this PR
##########
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.
##########
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:
Thanks for the explanation. I agree that we shouldn't try to address that in this PR
##########
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.
##########
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:
Thanks for the explanation. I agree that we shouldn't try to address that in this PR
##########
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.
##########
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:
Thanks for the explanation. I agree that we shouldn't try to address that in this PR
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r487566328
##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
##########
@@ -57,6 +57,17 @@ protected final double evalDouble(double left, double right)
// Use Double.compare for more consistent NaN handling.
return Evals.asDouble(Double.compare(left, right) < 0);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
+ return implicitCast;
Review comment:
what should be the expected output type here if the left output type is a string and the right output type is a double?
As per `org.apache.druid.math.expr.BinaryEvalOpExprBase#eval` it seems, the runtime output type will be `double` . As per `ExprType.autoTypeConversion` it seems to be `string`.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #10370:
URL: https://github.com/apache/druid/pull/10370#issuecomment-690755046
This PR will cause soft conflicts (which will probably not show up as merge conflicts) with any open PRs that are adding any new `Expr`, `ExprMacro`, `Function`, `ApplyFunction`, since they will need to include a `getOutputType` implementation after this PR is merged.
at minimum these open PRs would be affected:
#10084
#10230
#10350
and likewise if any of these go in I will need to fixup this branch before it can be merged.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r485989681
##########
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:
Maybe? This case can't actually happen right now, all cases are actually covered in previous clauses so this line should currently never be hit. I was sort of starting to imagine ahead to a world where `ExprType` and `ValueType` are unified and things like complex types could appear in a type conversion logic block similar to this. I sort of expect this area of the code to be somewhat volatile in the near future as I work out some things about type handling.
Usage wise, this method is basically only used to compute the output type given 2 input types, so it at least partially makes sense to return null if we can't determine the output type. I can see an argument for this being an exception as well (as well as either argument being null). `implicitCast` isn't a great name for this method either, I should probably call it something else, maybe something like `implicitTypeConversion` or .. something.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r488075026
##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
##########
@@ -57,6 +57,17 @@ protected final double evalDouble(double left, double right)
// Use Double.compare for more consistent NaN handling.
return Evals.asDouble(Double.compare(left, right) < 0);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
+ return implicitCast;
Review comment:
> This PR is setting up for vectorized expressions, which are going to be strongly typed before processing, which I think makes this inconsistent/confusing/magical behavior not necessary.
can you elaborate more on this? How will that look like?
While I understand the right behavior is debatable, it may still be best to keep the type in sync with eval even if it's not very intuitive. It may require that the logic of figuring out the type is not reusable as a function `ExprType.autoTypeConversion` and may have to be written differently for some operators. We can, later on, change the different functions, fixing both the `eval` and `getOutputType` together. But till then, they will be in sync.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r487868191
##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
##########
@@ -57,6 +57,17 @@ protected final double evalDouble(double left, double right)
// Use Double.compare for more consistent NaN handling.
return Evals.asDouble(Double.compare(left, right) < 0);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
+ return implicitCast;
Review comment:
>so something like 2.0 + '3.1' = 5.1
That might not have been a great example, this might normal behavior in some other databases 😅. I'm going to have a closer read over https://www.postgresql.org/docs/9.0/typeconv-oper.html and https://www.postgresql.org/docs/9.0/typeconv-func.html and maybe reference some other docs too and see if might make sense to different versions of this implicit conversion function for different contexts.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r486762521
##########
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.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r488172223
##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
##########
@@ -57,6 +57,17 @@ protected final double evalDouble(double left, double right)
// Use Double.compare for more consistent NaN handling.
return Evals.asDouble(Double.compare(left, right) < 0);
}
+
+ @Nullable
+ @Override
+ public ExprType getOutputType(InputBindingTypes inputTypes)
+ {
+ ExprType implicitCast = super.getOutputType(inputTypes);
+ if (ExprType.STRING.equals(implicitCast)) {
+ return ExprType.LONG;
+ }
+ return implicitCast;
Review comment:
>can you elaborate more on this? How will that look like?
Ah, its going to be based on using these methods, just the processors will be specialized to deal with the correct type based on the set of input types. Since the output type information isn't used for non-vectorized expressions, I'm trying to model this as the change I want to see in the world and will ensure that this matches the behavior of the vectorized expressions, but I've gone ahead and split operator from function auto conversion and changed it to match existing behavior for now in case it is necessary, and can always consolidate them again in the future.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #10370: add computed Expr output types
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #10370:
URL: https://github.com/apache/druid/pull/10370
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org