clintropolis commented on code in PR #13809:
URL: https://github.com/apache/druid/pull/13809#discussion_r1119934374
##########
processing/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java:
##########
@@ -331,7 +331,9 @@ public ExprEval eval(ObjectBinding bindings)
return ExprEval.ofLongBoolean(false);
}
ExprEval rightVal;
- if (NullHandling.sqlCompatible() || Types.is(leftVal.type(),
ExprType.STRING)) {
+ // null values can (but not always) appear as string typed
+ // so type isn't necessarily string unless value is non-null
+ if (NullHandling.sqlCompatible() || (Types.is(leftVal.type(),
ExprType.STRING) && leftVal.value() != null)) {
Review Comment:
I initially made this change before i added `valueOrDefault`, because in a
situation where expressions are evaluated without the type information to tell
us that a null is a long (such as ingestion time transforms), an expression
like `null && 1` with the left side is a null value it will be evaluated as a
`STRING` typed expression, and follow the string/sql compatible rules, in
default value mode meaning `value()` spits out `null`. if the expression was `1
&& null` however, it would be evaluated with long rules, and spit out `0` in
default value mode.
None of this really matters now though, since callers should be using
`valueOrDefault()` after the changes in this patch, instead of `value()`, which
produces equivalent results, so I will switch it back and switch the tests
around null constants to use 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
