[GitHub] [druid] clintropolis commented on a diff in pull request #13809: move numeric null value coercion out of expression processing engine

["clintropolis (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@druid.apache.org

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