[ 
https://issues.apache.org/jira/browse/CALCITE-2438?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16571211#comment-16571211
 ] 

Julian Hyde commented on CALCITE-2438:
--------------------------------------

Please, please don't do anything major to {{RexSimplify}} as part of this case 
(or this PR). This PR is almost done, so let's commit it. It doesn't make 
things perfect, but it makes things better.

We have other work planned to RexSimplify: CALCITE-2338 and CALCITE-2326. Could 
you take on the latter?

bq. 1) Avoid multiple passes over the same expressions.

This is mostly unavoidable. The planner fires rules, and the rules are 
independent. Each rule wants to apply some basic simplifications, and relies on 
previous rules having applied the same simplifications.

bq. 2) I would like to rework {{unknownAsFalse}} into support for {{IS TRUE}}. 
Current support for {{unknownAsFalse}} is hard to follow, and it looks like 
IS_TRUE(expr) allows for the same optimizations. In other words, 
"unknownAsFalse" would become a silent adding of IS_TRUE on top of the input 
expression. Then rules like {{OR(x, y) IS TRUE ==> x IS TRUE or y IS TRUE}} 
would push {{IS TRUE}} downstream.

I am skeptical whether this approach will work. If a call to {{simplify(e, 
unknownAsFalse)}} has to, as its first step, rewrite e, then this approach is 
not viable. (I tried something similar a year or so ago.)

bq. 3) I would like to "reorder" predicates in a defined order (e.g. rewrite 
{{5=x ==> x=5}} or {{(x=1 or y=2) and (z=3)}} ==> {{(z=3) and (x=1 or y=2)}}. 
It looks like adding {{int nodeCount;}} to {{RexNode}} would be helpful to rank 
rex nodes.

This is a major change. You should log a JIRA case and start a discussion 
around it.

bq. 4) There are other bugs identified by expression fuzzer (like 
[https://github.com/apache/calcite/pull/776/commits/980f491323fcf61f5c966120870b3442cae41c23]or
 
[https://github.com/apache/calcite/pull/776/commits/f2fba02f02cfa6b094e67e37da6be2ee19861276]),
 so it does not make much sense in committing {{isAlwaysTrue}} only.

Thanks for finding these. But as I said above, the existence of bugs doesn't 
mean we should not commit the fix for this one.

> RexCall#isAlwaysTrue  return incorrect result
> ---------------------------------------------
>
>                 Key: CALCITE-2438
>                 URL: https://issues.apache.org/jira/browse/CALCITE-2438
>             Project: Calcite
>          Issue Type: Bug
>          Components: core
>    Affects Versions: 1.17.0
>            Reporter: pengzhiwei
>            Assignee: Julian Hyde
>            Priority: Critical
>         Attachments: 屏幕快照 2018-08-02 下午7.26.20.png, 屏幕快照 2018-08-02 
> 下午7.34.26.png
>
>
> In the expression as followed:
> {code:java}
> ((sal IS NULL) IS NOT NULL) IS FALSE
> {code}
> The RexCall#isAlwaysTrue return true,however the correct answer is false.
> I find the reason is that there is a wrong logic in the  isAlwaysTrue,when 
> getKind() is IS_NOT_FALSE、IS_FALSE and IS_NOT_FALSE :
> !屏幕快照 2018-08-02 下午7.34.26.png!
> Can you have a check for me,thanks!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to