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 > > > > >
