kgyrtkirk commented on code in PR #15552:
URL: https://github.com/apache/druid/pull/15552#discussion_r1425966990
##########
processing/src/main/java/org/apache/druid/math/expr/ExpressionTypeConversion.java:
##########
@@ -55,6 +55,9 @@ public static ExpressionType autoDetect(ExprEval eval,
ExprEval otherEval)
{
ExpressionType type = eval.type();
ExpressionType otherType = otherEval.type();
+ if (type == otherType && eval.value() != null && otherEval.value() !=
null) {
Review Comment:
sure - I was too cautious...if it will be `null` ; the type will be `String`
as per the comment above the method - so it will be only `==` if the other is
also a `String`
```
In this mode, null values are {@link ExprType#STRING} typed, despite
potentially coming from an underlying numeric column, or when an underlying
column was completely missing and so all values are null
```
regarding those lines:
if `eval.value() == null` ; after the 1st line `type = otherType` ;
independently from the the 2nd line that will mean that effectively
`numeric(otherType, otherType)` will be called...
```
if(eval.value() == null) {
return otherType;
}
if(otherEval.value() == null) {
return type;
}
return numeric(type, otherType);
```
which made me wonder... can `ExpressionType.getType() == COMPLEX` for this
method? if it could this change could alter its behaviour - as earlier `DOUBLE`
was returned in that case
to avoid changing that behaviour - for now I've changed it to:
```java
if (type == otherType && type.getType().isPrimitive()) {
return type;
}
```
should these pass / fail / or shouldn't exists at all?
```java
final ExprEval<?> complexEval =
ExprEval.ofComplex(ExpressionType.UNKNOWN_COMPLEX, new Object());
Assert.assertEquals(ExpressionType.DOUBLE,
ExpressionTypeConversion.autoDetect(longEval, complexEval));
Assert.assertEquals(ExpressionType.DOUBLE,
ExpressionTypeConversion.autoDetect(doubleEval, complexEval));
Assert.assertEquals(ExpressionType.DOUBLE,
ExpressionTypeConversion.autoDetect(arrayEval, complexEval));
Assert.assertEquals(ExpressionType.DOUBLE,
ExpressionTypeConversion.autoDetect(complexEval, complexEval));
```
--
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]
