vlsi commented on a change in pull request #1818: [CALCITE-3809] RexSimplify
simplifies nondeterministic function incorrectly
URL: https://github.com/apache/calcite/pull/1818#discussion_r384444060
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -323,7 +323,7 @@ private RexNode simplifyComparison(RexCall e, RexUnknownAs
unknownAs) {
// Simplify "x <op> x"
final RexNode o0 = operands.get(0);
final RexNode o1 = operands.get(1);
- if (o0.equals(o1)) {
+ if (o0.equals(o1) && RexUtil.isDeterministic(o0)) {
Review comment:
>Do they ruin the performance of RexSimplify for valid deterministic cases?
I don't think so.
Do you have proof of that?
Once again: adding `RexUtil.isDeterministic` inside every RexSimplify method
would easily produce O(N^2) behaviour.
So I see the following ways to proceed:
1) You add a single top-level check that walks over the expression tree and
just bails out in case it sees a non-deterministic expression
2) You provide verifiable proof that your change never results in O(N^2)
behaviours
3) You ignore my reviews and commit as is
I agree it is important to fix the functional issue "wrong results when a
non-deterministic function is present". However, it is not acceptable to solve
a functional issue by dramatically slowing down the processing, especially,
when the slowdown affects all the cases.
I would rather accept a trivial and understandable solution like `skip
simplification if non-deterministic is present` rather than an arcane `scatter
RexUtil.isDeterministic all over the place until it works`.
If you want to implement a clever approach (e.g. that would simplify
`AND(rand()=rand(), false, false)` to `AND(rand()=rand(), false)`) it is fine.
However, the implementation should still be linear with regard to the
expression tree size.
PS. Of course, `RexSimplify` might contain ill calls to
`RexUtil.isDeterministic`. That does not automatically allow adding new slow
`RexUtil.isDeterministic` checks all over the place. I would consider the
existing calls to be bugs that need to be fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
