clintropolis commented on code in PR #13809:
URL: https://github.com/apache/druid/pull/13809#discussion_r1119958152
##########
processing/src/main/java/org/apache/druid/math/expr/ExprEval.java:
##########
@@ -743,25 +749,37 @@ private NumericExprEval(@Nullable Number value)
@Override
public final int asInt()
{
+ if (value == null) {
+ assert NullHandling.replaceWithDefault();
+ return 0;
+ }
return value.intValue();
}
@Override
public final long asLong()
{
+ if (value == null) {
+ assert NullHandling.replaceWithDefault();
+ return 0L;
+ }
return value.longValue();
}
@Override
public final double asDouble()
{
+ if (value == null) {
+ assert NullHandling.replaceWithDefault();
+ return 0.0;
+ }
return value.doubleValue();
}
@Override
public boolean isNumericNull()
Review Comment:
heh, so here it sort of is important for the way things work (right now)
with non-vectorized expressions because the contract of the method is sort of
like that of the `ColumnValueSelector` `isNull` method - that if it returns
false then the `asLong` and such primitive returning methods will return a
valid value.
`StringExprEval` for example implements these methods as best effort
conversions, so `StringExprEval("100")` returns false for `isNumericNull` and
`asLong` returns the value 100L in both default value and sql compat modes.
`StringExprEval("x")` returns true for `isNumericNull` if in sql compatible
mode, but the `value()` is not in fact `null`, so callers should know that the
value of this is `null` in sql compatible mode instead of calling `asLong` to
get 0. In default value mode, `isNumericNull` returns false and so no one cares
if the string was a real number or not, if it wasn't it was zero.
Things that do not want to use these number methods can just check the
result of `value()` directly.
At least for the way things currently work, this is actually probably a more
accurate name than `isNull`, and arguably the `ColumnValueSelector` method
should probably be renamed to match this one 😛 . The only things that check
`isNull` on `ColumnValueSelector` are things that want to use the primitive
numeric getters, object selectors often do not implement it since callers are
currently expected to call `getObject` and check that it is `null` per the
current javadoc contract in `BaseNullableColumnValueSelector`.
I would be in favor of changing this contract into making a correct
implementation of `isNull` a requirement for all `ColumnValueSelectors`, but I
totally don't want to do that in this PR, and it would require I think
splitting up anything that does type coercion which might produce nulls into
separate selectors (and expressions to more liberally explicitly cast values to
different types instead of use these implicit conversions which some of them
have)
--
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]
