Haisheng, I hear you. I agree that major changes to rules will require new rule classes (not merely sub-classes). People should copy-paste, refactor, and all that good stuff. But I think there are a lot of cases where we need to make minor changes to rules (there are many of these in the code base already), and this change will help.
I have logged https://issues.apache.org/jira/browse/CALCITE-3923 and am going to start working on a prototype. When we have a prototype we will be able to assess how big an impact the API change will have. (E.g. whether it will be a breaking change.) Julian On Sat, Mar 14, 2020 at 8:22 PM Haisheng Yuan <h.y...@alibaba-inc.com> wrote: > > I don't think it is worth the refactoring. People who want to customize the > rule, in most cases, won't be satisfied by a different parameter, they most > likely still need to rewrite (copy & paste) the rule with some slightly their > own logic. For many Calcite users, the rule is not reusable even with > flexible configurations. > > - Haisheng > > ------------------------------------------------------------------ > 发件人：Stamatis Zampetakis<zabe...@gmail.com> > 日 期：2020年03月14日 22:54:04 > 收件人：<dev@calcite.apache.org> > 主 题：Re: [DISCUSS] Refactor how planner rules are parameterized > > Hello, > > Apologies for the late reply but I just realised that I had written the > mail and never pressed the send button. > > I think it is a nice idea and certainly a problem worth addressing. If I > understood well you're thinking something like the current constructor of > the RelBuilder [1] that accepts a Context parameter. Indeed it seems that > with this change even rules that are not designed to be configured can be > changed much more gracefully (without adding new constructors and breaking > changes). > > On the other hand, some of the advantages that you mention can also be > turned into disadvantages. For instance, copying a rule without knowing the > values of the other parameters is a bit risky and might be harder to reason > about its correctness. Moreover, private constructors, final classes, etc., > are primarily used for encapsulation purposes so allowing the state of the > rule escape somehow breaks the original design of the rule. > > Another problem with respect to rules is cross convention matching and > transformations [2]. Many rules should not fire for operands that are in > different conventions; a typical example that comes in my mind is > FilterProjectTransposeRule [3]. In the same spirit most rules should not > generate mixed convention transformations. Although a different problem, I > am mentioning it here since it could affect the design of the new API. > > Best, > Stamatis > > [1] > https://github.com/apache/calcite/blob/f11115a2fe9e360f38910f112288581040e0ced5/core/src/main/java/org/apache/calcite/tools/RelBuilder.java#L155 > > [2] > https://lists.apache.org/thread.html/da1860f99f8bfd6ec7d26626c428ce1c55480e7c61ae7f83060a40c2%40%3Cdev.calcite.apache.org%3E > [3] > https://github.com/apache/calcite/blob/7c27b147414c64505fa33c947100ece094caa15c/core/src/main/java/org/apache/calcite/rel/rules/FilterProjectTransposeRule.java#L57 > > On Thu, Feb 20, 2020 at 9:20 PM Michael Mior <mm...@apache.org> wrote: > > > This sounds reasonable to me. It also sounds like we could make this > > backwards compatible by retaining (but deprecating) the existing > > constructors and factory methods that will no longer be needed. > > -- > > Michael Mior > > mm...@apache.org > > > > Le jeu. 20 févr. 2020 à 13:11, Julian Hyde <jh...@apache.org> a écrit : > > > > > > I have an idea for a refactoring to RelOptRule. I haven’t fully thought > > it through, but I’m going to sketch it out here to see whether folks agree > > about the problems/solutions. > > > > > > It will be a breaking change (in the sense that people will have to > > change their code in order to get it to compile) but relatively safe (in > > that once the code compiles, it will have the same behavior as before). > > Also it will give Calcite developers and users a lot more flexibility going > > forward. > > > > > > The problem I see is that people often want different variants of > > planner rules. An example is FilterJoinRule, which has a 'boolean smart’ > > parameter, a predicate (which returns whether to pull up filter > > conditions), operands (which determine the precise sub-classes of RelNode > > that the rule should match) and a relBuilderFactory (which controls the > > type of RelNode created by this rule). > > > > > > Suppose you have an instance of FilterJoinRule and you want to change > > ‘smart’ from true to false. The ‘smart’ parameter is immutable (good!) but > > you can’t easily create a clone of the rule because you don’t know the > > values of the other parameters. Your instance might even be (unbeknownst to > > you) a sub-class with extra parameters and a private constructor. > > > > > > So, my proposal is to put all of the config information of a RelOptRule > > into a single ‘config’ parameter that contains all relevant properties. > > Each sub-class of RelOptRule would have one constructor with just a > > ‘config’ parameter. Each config knows which sub-class of RelOptRule to > > create. Therefore it is easy to copy a config, change one or more > > properties, and create a new rule instance. > > > > > > Adding a property to a rule’s config does not require us to add or > > deprecate any constructors. > > > > > > The operands are part of the config, so if you have a rule that matches > > a EnumerableFilter on an EnumerableJoin and you want to make it match an > > EnumerableFilter on an EnumerableNestedLoopJoin, you can easily create one > > with one changed operand. > > > > > > The config is immutable and self-describing, so we can use it to > > automatically generate a unique description for each rule instance. > > > > > > Julian > > > > > > [1] > > https://github.com/apache/calcite/blob/5fa41609cb0fe310a0a11d86319d861423850a36/core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java#L93 > > < > > https://github.com/apache/calcite/blob/5fa41609cb0fe310a0a11d86319d861423850a36/core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java#L93 > > > > > > > > >
