korlov42 commented on code in PR #7768:
URL: https://github.com/apache/ignite-3/pull/7768#discussion_r2938469031
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/ConverterUtils.java:
##########
@@ -221,10 +221,18 @@ private static Expression convertToTimestamp(Expression
operand, RelDataType tar
methodName = "toTimestampLtzExact";
}
+ // Box primitive operand to ensure the null-safe Object overload is
selected,
+ // and RexImpTable builds a correct condition in genValueStatement()
method
+ // if type is long then the condition: is input_isNull ? 0 :
<toTimestampExact>
+ // Otherwise, when type is Long, the condition is is input_isNull ?
null : <toTimestampExact>
+ Expression safeOperand = Primitive.is(operand.getType())
Review Comment:
tbh this looks more than WA for me.
I think, the root cause is
`org.apache.ignite.internal.sql.engine.exec.exp.RexImpTable.AbstractRexCallImplementor#genValueStatement`
method. It's supposed to generate expression evaluation code guarded with null
check for input parameters. In my opinion, it must return `NULL` (and no other
values) according to `nullPolicy`. So, the point of interest is this particular
part:
```
final Expression valueExpression =
Expressions.condition(condition,
getIfTrue(convertedCallValue.getType(), argValueList),
convertedCallValue);
```
The method `getIfTrue` is really suspicious. Neither method name nor its
parameters does make any sense in the given context. More over, it is deciding
what default to return based on return type of the intermediate computation and
not return type of the expression. Hence, I would rather fix this condition.
Does it make sense?
--
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]
