Closed clintropolis closed 4 days ago
I think it would be great to have consistency check testcase which ensures that from now on the contract that least
should behave the same in non-vectorized mode and in vectorized mode .... (or that's already there with some trick I've missed?)
I understand that this patch fixes a consistency issue - but I'm a bit confused to see that it will be allowed to have DOUBLE
typed null
for the least
function; shouldn't others like +
also follow the same principle?
for example: with the proposed changes the following testLeast1()
will pass; however testAdd
will not.
@Test
public void testLeast1()
{
ObjectBinding a = new SingleInputBindings(ExpressionType.DOUBLE);
ExpressionType t1 = Parser.parse("least(1.0 , 2.0)", ExprMacroTable.nil()).eval(a).type();
ExpressionType t2 = Parser.parse("least(1.0 , null)", ExprMacroTable.nil()).eval(a).type();
assertEquals(t1, t2);
}
@Test
public void testAdd()
{
ObjectBinding a = new SingleInputBindings(ExpressionType.DOUBLE);
ExpressionType t1 = Parser.parse("1.0 + 2.0", ExprMacroTable.nil()).eval(a).type();
ExpressionType t2 = Parser.parse("1.0 + null", ExprMacroTable.nil()).eval(a).type();
assertEquals(t1, t2);
}
is that okay? couldn't the same happen for +
?
I think it would be great to have consistency check testcase which ensures that from now on the contract that least should behave the same in non-vectorized mode and in vectorized mode .... (or that's already there with some trick I've missed?)
Technically neither of these functions are vectorized yet and just rely on the fallback stuff introduced in #16366. There are such tests that fit this purpose, but none of the fallback stuff has been added here https://github.com/apache/druid/blob/master/processing/src/test/java/org/apache/druid/math/expr/VectorExprSanityTest.java https://github.com/apache/druid/blob/master/sql/src/test/java/org/apache/druid/sql/calcite/SqlVectorizedExpressionSanityTest.java because its basically like.. all the functions i guess.
I understand that this patch fixes a consistency issue - but I'm a bit confused to see that it will be allowed to have DOUBLE typed null for the least function; shouldn't others like + also follow the same principle?
So, the greatest
and least
functions do not follow the same behavior as math operations, for whatever reason a null argument does not result in the output of the function being null the same way add does. See comment in https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/math/expr/Function.java#L635.
The reason the output type is double in your first test is because the result is 1.0, while the reason it is string in the second one is a sort of legacy behavior of non-vectorized expressions where the output type of null values is STRING
. Generally, when dealing with ExprEval
, the type
is ignored if the value is null because it isn't trusted. Vectorized expressions on the other hand will create a 'null' vector of whatever the inferred output type of the expression is, so that addition example when vectorized has an output type of double, with null value vectors.
one more question: is it true that a null will always be of type STRING?
Null constants always will be currently, while null values from columns will be whatever column type they came from (e.g. long column make ExprEval
with ExpressionType.LONG
with null values). The exception to from columns is in cases where we have no type information and are using expressions, which discover the types on the fly with ExprEval.bestEffortOf
, in which case they also will be string like constants.
Description
Fixes a flaw in
greatest
andleast
functions non-vectorized expression processing to ignorenull
valued arguments for determining the output type. For example, given an expression likeleast(1.0, 1, null)
, thegetOutputType
method correctly choosesDOUBLE
as the output type, however during non-vectorized processing, we incorrectly consider it, which ends up with the defaultSTRING
output type, resulting in incorrectly using theSTRING
comparator instead of the double comparator as expected. This can cause odd effects and incorrect results because values like1.0
and1
are considered distinct when using the string comparator, while the same when using a numeric comparator.The added tests fail without the changes in this PR.
This PR has: