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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
