imply-cheddar commented on code in PR #13809:
URL: https://github.com/apache/druid/pull/13809#discussion_r1119647915
##########
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 feel like I'm not understanding this logic, but it seems like previously
you were doing null checks if it was a String. Now, you will only do null
checks if it is a String *and* the left-hand side is not null? This seems to
be trying to coerce for boolean operations, if the left side is numerical but
also not null, it seems like we should have semantics that are like `if == 0
then false, else true`. But, I think that this will just ignore it if it's not
String?
##########
processing/src/main/java/org/apache/druid/math/expr/vector/VectorProcessors.java:
##########
@@ -242,6 +270,12 @@ public void processIndex(Object[] strings, long[] longs,
boolean[] outputNulls,
return (ExprVectorProcessor<T>) processor;
}
+ /**
+ * Creates an {@link ExprVectorProcessor} for the 'isnull' function, that
produces a "boolean" (long) typed output
+ * vector with values set to 1 if the input value was null or 0 if it was
not null.
Review Comment:
am I correct in understanding that this creates a `long[]`?
##########
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();
Review Comment:
Do we really need the assertions?
##########
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:
I know this is a comment on unchanged code, but wondering how you feel about
renaming this to just `isNull`? I've never quite understand why the `Numeric`
part was important.
##########
processing/src/main/java/org/apache/druid/math/expr/ExprEval.java:
##########
@@ -1042,11 +1078,12 @@ public final ExprEval castTo(ExpressionType castTo)
case STRING:
return this;
case ARRAY:
+ final Number number = computeNumber();
switch (castTo.getElementType().getType()) {
case DOUBLE:
- return ExprEval.ofDoubleArray(value == null ? null : new
Object[] {computeDouble()});
+ return ExprEval.ofDoubleArray(value == null ? null : new
Object[] {number != null ? number.doubleValue() : null});
Review Comment:
nit: invert the ternary to be `== null`?
##########
processing/src/main/java/org/apache/druid/math/expr/vector/VectorProcessors.java:
##########
@@ -778,6 +842,15 @@ public ExprEvalVector<long[]> asEval()
);
}
+ /**
+ * Creates an {@link ExprVectorProcessor} for the logical 'and' operator,
which produces a long typed vector output
+ * with values set by the following rules:
+ * true/true -> true (1)
+ * true/null, null/true, null/null -> null
+ * false/null, null/false -> false (0)
Review Comment:
Why does `null` null out a `true`, but not `null` out a `false`!?!?!?!?!?
Is that some SQL-ism to handling nulls?
##########
processing/src/main/java/org/apache/druid/math/expr/vector/VectorProcessors.java:
##########
@@ -350,6 +384,12 @@ public ExpressionType getOutputType()
return (ExprVectorProcessor<T>) processor;
}
+ /**
+ * Creates an {@link ExprVectorProcessor} for the 'isnotnull' function, that
produces a "boolean" (long) typed output
Review Comment:
Same here, a `long[]`?
##########
processing/src/main/java/org/apache/druid/math/expr/ExprEval.java:
##########
@@ -771,7 +789,7 @@ private static class DoubleExprEval extends NumericExprEval
private DoubleExprEval(@Nullable Number value)
{
- super(value == null ? NullHandling.defaultDoubleValue() : (Double)
value.doubleValue());
+ super(value == null ? null : value.doubleValue());
Review Comment:
Does the `.doubleValue()` add anything here? It looks like it's storing a
reference to a Number anyway? I guess it's a forced cast?
--
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]
