[CALCITE-3809] RexSimplify simplifies nondeterministic function incorrectly

[chunweilei]

JIRA: https://issues.apache.org/jira/browse/CALCITE-3809

[vlsi]

Is deep check required here? Doesn't it produce O(N^2) behaviors?

[chunweilei]

IMO, yes, it is necessary here. It cannot be simplified if there is nondeterministic call, i.e. abs(abs(abs(rand())))=abs(abs(abs(rand()))).

[vlsi]

Is the implementation linear with regards to the number of nodes in the tree?

[chunweilei]

Yes for RexUtil.isDeterministic. Are you worried about the performance of RexUtil.isDeterministic? Or do you think it is not the right solution?

[vlsi]

Suppose the expression is always deterministic. It looks like "deep isDeterministic" would fire too often, and it could ruin the performance of RexSimplify for perfectly valid deterministic cases. That is not how the thing should work.

If I were you, I would try to answer the following question:

1. Is it legitimate to simplify at all if non-deterministic functions are known to exist in the tree.
   For instance: AND(rand(), true, true, rand())
2. What do we want to do with Requested merge of OPTIQ-247, 324,326,325,323,322,321 #1 (e.g. keep unsimplified, or simplify deterministic subexpressions)
3. Use a single pass to find non-deterministic expressions. A single pass is enough to tell if the expression is fully deterministic or not. If it is, then you don't need to check for non-deterministic expressions later.

I don't like adding non-trivial logic inside RexSimplify, because it is really hard to tell if the resulting performance is linear or not.

[chunweilei]

@vlsi do we reach a consensus about this point? Appreciate your review.

[vlsi]

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.

[chunweilei]

Do you notice that RexUtil.isDeterministic check is only called when simplifying "x <op> x ?
Will it have such a significant impact as you said? I have a doubt about it.

[vlsi]

How about ((rand()=rand())=(rand()=rand()))=((rand()=rand())=(rand()=rand())) denial of service attack?

I don't really want to spend your time, so please proceed with whatever way you feel is right.

[chunweilei]

I believe we're not arguing. Both of us are trying to find the best way. Appreciate your time and attention.