This is a change that most likely will have impact on many projects so I think it deserves a bit more discussion. Given that 1.22 should be out quite soon and at the same time this period many people are on holidays it would seem better to at least postpone the merge for 1.23.
In terms of code, I would prefer if the changes were part of another class (i.e., RexNormalizer?) that would leave the RexNode hierarchy unchanged. Many projects use Calcite as a library, combining components the way they like. To this end the approach of a separate class seems more appropriate. This seems inline with the spirit of the other classes that we have for simplifying, flattening etc. Best, Stamatis On Tue, Dec 31, 2019, 2:47 PM Vladimir Sitnikov <[email protected]> wrote: > Feng>Maybe we can go a step further to provide configuration interfaces, > which > Feng>enable users to determine whether to enable a specified optimization > > It might be OK to add kill-flags so added features can be disabled for > debugging purposes. > However, I would prefer to avoid adding options that will be used in > non-default values in production systems. > > For instance, I think https://github.com/apache/calcite/pull/1703 (Reorder > RexCall predicates to a canonical form) > is ready for merge. > > It improves planning performance across many tests, and it a small > patch: +255 -84. > > The patch is an improvement, there are very tiny drawbacks, so I'm inclined > to commit the change. > Just in case you did not know > RelOptUtil#toString(org.apache.calcite.rel.RelNode) output is preserved by > the patch. > In other words, all the tests that use RelOptUtil#toString for dumping > plans will likely work as-is. > > I have added a system property calcite.enable.rexnode.digest.normalize > (default=true) so the feature can be disabled > if someone thinks it causes issues (e.g. if there's a suspicion it slows > down the planning for some reason). > However, the full Calcite suite is executed with the default value only, so > I would not recommend using normalize=false in production. > > Vladimir >
