mihaibudiu commented on code in PR #5228:
URL: https://github.com/apache/calcite/pull/5228#discussion_r3874747864
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1545,15 +1556,25 @@ private static List<RexNode>
toCaseOperands(List<CaseBranch> branches) {
/**
* Decides whether it is safe to flatten the given CASE part into ANDs/ORs.
*/
- enum SafeRexVisitor implements RexVisitor<Boolean> {
- INSTANCE;
+ private static class SafeRexVisitor implements RexVisitor<Boolean> {
Review Comment:
is this javadoc still accurate?
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1614,16 +1635,23 @@ private boolean isSafe(RexCall call, boolean deep) {
SqlKind sqlKind = call.getKind();
SqlOperator sqlOperator = call.getOperator();
- if (SqlKind.CHECKED_ARITHMETIC.contains(sqlKind)) {
- // Checked arithmetic throws on overflow, so it is only safe when the
- // arithmetic is never performed, i.e. when an operand is NULL.
+ if (SqlKind.CHECKED_ARITHMETIC.contains(sqlKind)
Review Comment:
I am not thrilled about this: we are leaking details about specific
operators in a class (RexSimplify) which should be rather general. This was
already there in the previous solution, but this makes it worse.
On the other hand I am not sure I have a better proposal.
The safety should really be a property that a RexCall can report - based on
inspecting it's argument types.
--
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]
