I agree with this plan. We already have ~70 commits in master so it is good enough to go for a release
On Thu, Jul 2, 2020 at 12:27 AM Julian Hyde <[email protected]> wrote: > PS If we take that approach, let's release 1.24 soon. CALCITE-3923 is > a big PR and bit-rot will set in if it lingers too long. > > On Wed, Jul 1, 2020 at 3:23 PM Julian Hyde <[email protected]> wrote: > > > > Stamatis, > > > > Do you mean we should put into the next release (1.24) just a few > > lines '@Deprecated // to be removed before 1.25; use xxx instead' > > tagging the breaking changes, and then make the breaking changes in > > master immediately after 1.24? > > > > I would support that approach. We could deal with > > https://issues.apache.org/jira/browse/CALCITE-4079 at the same time. > > > > For the record, there won't be many breaking changes in > > https://issues.apache.org/jira/browse/CALCITE-3923. (Just a few > > constants that we have to remove because of class-loading issues.) But > > there will be a lot of things deprecated because they have been moved > > to a "better" place. > > > > On Sun, Jun 28, 2020 at 4:08 PM Stamatis Zampetakis <[email protected]> > wrote: > > > > > > Indeed nice explanation Julian, thanks! > > > > > > Regarding the transition one thing that might help is to try to put > the new > > > model in a small release without other breaking changes. > > > We could get 1.24 out without the refactoring and soon afterwards > release > > > 1.25. > > > > > > Given that the community has been rather silent on this I guess that > people > > > don't really mind if it goes as is in the next release so I may be over > > > complicating things for no reason. > > > In the end, we have also committed features with bigger impact with no > > > special treatment. > > > > > > Best, > > > Stamatis > > > > > > > > > On Fri, Jun 19, 2020 at 11:37 PM Haisheng Yuan <[email protected]> > wrote: > > > > > > > Thank you for the detailed explanation, Julian. > > > > > > > > I will step aside, let you and other community members decide. > > > > Anyway, my comment is not blocking. > > > > > > > > Thanks, > > > > Haisheng > > > > > > > > On 2020/06/17 23:12:40, Julian Hyde <[email protected]> wrote: > > > > > "I prefer the KISS principle" is a bit unfair. What I'm advocating > is > > > > > also simple. But maybe from a different perspective. > > > > > > > > > > I am looking at it from the perspective of people who use rules, > and > > > > > compose them into rule sets to perform particular optimizations. I > > > > > believe this is the most important perspective to focus on. These > > > > > people are the key audience to serve. The corpus of re-usable, > > > > > composable rules may be Calcite's most important contribution. > > > > > > > > > > To these people, a rule is not a class but an object. It has a > > > > > particular signature (in terms of the pattern of RelNodes that it > > > > > matches), maybe one or two configuration parameters (e.g. > > > > > FilterJoinRule.smart controls whether to automatically strengthen a > > > > > LEFT join to INNER), and it has code to generate a new RelNode when > > > > > matched. > > > > > > > > > > What do I mean by 'object'? Think of how you use regular > expressions > > > > > in Java. You call java.util.regex.Pattern.compile("a[bc]*") and it > > > > > gives you a Pattern object. You do not know what fields are inside > > > > > that Pattern, you do not care whether the object you receive is a > > > > > Pattern or some sub-class of Pattern, but you know there are one or > > > > > two methods such as 'matcher(CharSequence)' that you can call. > > > > > > > > > > Our current API treats planner rules as classes. If you want to > > > > > customize a property of a rule (say make a FilterJoinRule with > > > > > smart=false, or change its RelBuilder, or make it match a > > > > > CassandraFilter rather than a LogicalFilter) then you have to make > a > > > > > new rule instance by calling the constructor. > > > > > > > > > > When you call the constructor, you have to supply ALL of the > > > > > properties, including operands, and including properties that you > > > > > don't know or care about. If someone in three months adds a new > > > > > property, the signature of the constructor will change, and you > will > > > > > have to go back and change your code. This is broken. > > > > > > > > > > Haisheng asked for evidence that the current system is broken. > > > > > https://issues.apache.org/jira/browse/CALCITE-3825 ("Split > > > > > AbstractMaterializedViewRule into multiple classes") is one > example. > > > > > In my company, that change literally broke our code because we had > > > > > changed properties of some rules. > > > > > > > > > > Another example is > https://issues.apache.org/jira/browse/CALCITE-3975 > > > > > ("Add options to ProjectFilterTransposeRule to push down project > and > > > > > filter expressions whole, not just field references"). With that > > > > > change, I broke my own code. I added two new arguments, 'boolean > > > > > wholeProject, boolean wholeFilter' to ProjectFilterTransposeRule's > > > > > constructor, deprecated the previous constructor, and created > quite a > > > > > few conflicts in another dev branch that I was working on. > > > > > > > > > > These kinds of problems are not uncommon. Because it is difficult > to > > > > > reconfigure rules, or evolve them by adding extra arguments, people > > > > > are more likely to copy-paste rule code into their own projects, > and > > > > > less like to contribute back. > > > > > > > > > > In most rules, operands are created evaluating a complex > expression - > > > > > made up of calls to static methods such as RelOptRule.operand - in > a > > > > > constructor as an argument to the base class constructor. This is a > > > > > code smell. Such code is hard to move elsewhere. > > > > > > > > > > We treat operands as immutable, but actually they are mutable. > Every > > > > > operand contains mutable fields 'solveOrder', 'ordinalInParent' and > > > > > 'ordinalInRule' that are assigned in the RelOptRule constructor. We > > > > > don't prevent you from passing the same operand to two different > rule > > > > > instances, but if you did, bad things would happen. Actually I > think > > > > > people would love to be able to say 'create a rule instance the > same > > > > > as this, but replacing LogicalFilter with CassandraFilter' but we > > > > > don't make it easy, and copying operands from one rule to another > is > > > > > certainly the wrong way to achieve it. > > > > > > > > > > Given all of these problems, the solution was to convert rules into > > > > > data objects. Those data objects are the new Config interface and > its > > > > > sub-interfaces. (I used an interface because the ImmutableBeans > > > > > framework makes it easy to create immutable data objects without > > > > > generating code or writing a lot of boilerplate code by hand.) > > > > > > > > > > These Config interfaces mean one extra class per rule, and about 5 > > > > > extra lines of source code. If you measure simplicity in lines of > > > > > code, then I suppose they make things a little less simple. > > > > > > > > > > The new way of building operands, using a builder that makes > > > > > call-backs for each nested operand, and has a method for each > > > > > attribute of an operand (e.g. predicate()) is also 'less simple' if > > > > > you count lines of code. But it is more elegant in that it can > > > > > (potentially) produce genuinely immutable operands, is strongly > typed, > > > > > and offers helpful code-completion when you use it in an IDE. > > > > > > > > > > I hope you now understand the problems I am trying to solve, and > why > > > > > this new approach is better. That said, this change isn't perfect. > We > > > > > can evolve this change to make the code more concise, or easier to > > > > > understand. > > > > > > > > > > Let's discuss some possible improvements. > > > > > > > > > > One possible improvement would be to stop using rule instances > > > > > (RelOptRule) and instead use rule config instances > > > > > (RelOptRule.Config). The planner could easily call Config.toRule() > > > > > when building a program. The sub-classes of RelOptRule would be > > > > > largely invisible (private sub-classes of the Config classes), and > > > > > that is appropriate, because all they provide is the > implementation of > > > > > the onMatch() method. There would be a one-time impact because a > lot > > > > > of types would change from RelOptRule to RelOptRule.Config. > > > > > > > > > > Another improvement would be to move around the rule constants. > > > > > FilterCorrelateRule.INSTANCE and > AggregateFilterTransposeRule.INSTANCE > > > > > would become the fields FILTER_CORRELATE and > > > > > AGGREGATE_FILTER_TRANSPOSE in a new class, LogicalRules. People > would > > > > > no longer need to reference the names of rule classes in their > code, > > > > > just the instances. And of course they can now easily customize > those > > > > > instances. > > > > > > > > > > I hope I have convinced you that rules need to become 'data > objects' > > > > > with a small amount of behavior, and that people need to be able to > > > > > customize them without calling their constructor or even knowing > their > > > > > precise class. > > > > > > > > > > I would love to hear suggestions for how we can make the > transition to > > > > > this new model as smooth as possible. > > > > > > > > > > Julian > > > > > > > > > > On Sun, Jun 14, 2020 at 9:15 PM Haisheng Yuan <[email protected]> > wrote: > > > > > > > > > > > > Druid adapter rules are not used by Druid. As far as I know, > only Hive > > > > uses these rules, I even think they should be moved to Hive project. > :) If > > > > any other projects other than Hive are using them, please let us > know. > > > > Basically, these Druid rules are not generally applicable. The PR > changed > > > > many built-in rules to specifically accommodate Druid adapter rules, > > > > especially with third operand that can match any operator, even > > > > EnumerableTableScan is legit. > > > > > > > > > > > > Most converter rules are not designed to be flexible, no one > would > > > > extend them. RelOptRule#operand() methods are deprecated, which > implies > > > > someday, they will be removed and migrated to the new Config. But > most down > > > > stream projects don't need to worry about the their rules' > extensibility > > > > and compatibility, because they can modify their own rules freely, > anytime. > > > > > > > > > > > > At the end of the day, we may find that only a small fraction of > > > > transformation rules need refactoring, e.g. > ProjectFilterTransposeRule. > > > > Note that even FilterProjectTransposeRule doesn't need refactoring, > we can > > > > safely remove copyFilter and copyProject, it is safe to always copy > them, > > > > do we really want to match physical operators and generate logical > > > > alternatives? > > > > > > > > > > > > Last but the most important, rule operands, IMO, should be out of > > > > Config's reach. How frequent do we need to change rule operands or > its > > > > matching class? IMO, operands of rule constructor should remain > unchanged, > > > > RelFactory and Rule description can also remain the same, or adapted > by > > > > Config silently without changing the rule itself, if there are no > other > > > > additional parameters. Other than that, everything can be put into > Config. > > > > Therefore I don't think RelOptNewRule is needed, because RelOptRule > should > > > > be able to integrate with Config seamlessly, without breaking > anything. > > > > > > > > > > > > All in all, I prefer the KISS principle, keep it simple, stupid. > > > > > > > > > > > > So my opinion is +0.5. > > > > > > > > > > > > Thanks, > > > > > > Haisheng > > > > > > > > > > > > On 2020/06/14 23:36:21, Stamatis Zampetakis <[email protected]> > wrote: > > > > > > > Hello, > > > > > > > > > > > > > > Thanks for putting this together Julian. > > > > > > > > > > > > > > I went quickly over the PR just to grasp better the idea and > here > > > > are some > > > > > > > first thoughts. > > > > > > > > > > > > > > I really like the fact that rules creation is now much more > uniform > > > > than > > > > > > > before. > > > > > > > I also like the fact that in some cases subclassing can be > avoided > > > > thanks > > > > > > > to the flexible configuration. As a fan of the Liskov > > > > > > > substitution principle I find it positive to be able to avoid > > > > classes such > > > > > > > as DruidProjectFilterTransposeRule although to be honest I > don't > > > > know why > > > > > > > these Druid rules impose those additional restrictions. > > > > > > > > > > > > > > On the other hand, I also feel that the new approach is a bit > harder > > > > to > > > > > > > follow than the previous one. The fact that we have more > extension > > > > points > > > > > > > gives more flexibility but at the same time complicates the > > > > implementation > > > > > > > a bit. I guess regular committers will not have much trouble to > > > > adapt to > > > > > > > the changes but for newcomers there may be more questions. For > > > > instance: > > > > > > > What do we do when we want to customize the behavior of an > existing > > > > rule? > > > > > > > * Use an existing config with different parameters. > > > > > > > * Extend the config. > > > > > > > * Extend the rule. > > > > > > > * Extend the config and the rule. > > > > > > > * Create a new rule (if yes under which interface RelOptRule or > > > > > > > RelOptNewRule). > > > > > > > > > > > > > > As Haisheng mentioned, the fact that every rule can be > configured > > > > with any > > > > > > > RelOptRuleOperand is also a point possibly deserving more > discussion. > > > > > > > Ideally, the API should be able to guide developers to pass the > > > > correct > > > > > > > configuration parameters and not fail at runtime. > > > > > > > > > > > > > > Overall, I have mixed feelings about the proposed refactoring > > > > (definitely > > > > > > > not something blocking), I guess cause I haven't seen as many > > > > use-cases as > > > > > > > Julian. > > > > > > > I'm curious to see what others think about the changes. It's a > pity > > > > to take > > > > > > > a decision based on the feedback of only two/three people. > > > > > > > > > > > > > > Best, > > > > > > > Stamatis > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sun, Jun 14, 2020 at 9:36 PM Haisheng Yuan < > [email protected]> > > > > wrote: > > > > > > > > > > > > > > > Hi Julian, > > > > > > > > > > > > > > > > Thanks for working on this. > > > > > > > > > > > > > > > > We haven't reached a consensus yet. > > > > > > > > > > > > > > > > Frankly speaking, I agree with what Stamatis said earlier. > > > > Flexibility > > > > > > > > doesn't come with no cost. Admittedly, with this patch, any > rule > > > > > > > > constructor refactoring won't need to deprecate old > constructors > > > > and break > > > > > > > > backward compatibility, however, it also makes the rule > definition > > > > much > > > > > > > > more verbose, less readable and understandable. IMHO, it > does more > > > > harm > > > > > > > > than good. > > > > > > > > > > > > > > > > Let's see how CassandraFilterRule becomes before and after > the > > > > change. > > > > > > > > > > > > > > > > Before this change: > > > > > > > > > > > > > > > > private static class CassandraFilterRule extends > RelOptRule { > > > > > > > > private static final CassandraFilterRule INSTANCE = new > > > > > > > > CassandraFilterRule(); > > > > > > > > > > > > > > > > private CassandraFilterRule() { > > > > > > > > super(operand(LogicalFilter.class, > > > > operand(CassandraTableScan.class, > > > > > > > > none())), > > > > > > > > "CassandraFilterRule"); > > > > > > > > } > > > > > > > > } > > > > > > > > > > > > > > > > After this change: > > > > > > > > > > > > > > > > private static class CassandraFilterRule > > > > > > > > extends RelOptNewRule<CassandraFilterRule.Config> { > > > > > > > > private static final CassandraFilterRule INSTANCE = > > > > > > > > Config.EMPTY > > > > > > > > .withOperandSupplier(b0 -> > > > > > > > > b0.operand(LogicalFilter.class) > > > > > > > > .oneInput(b1 -> > > > > b1.operand(CassandraTableScan.class) > > > > > > > > .noInputs())) > > > > > > > > .as(Config.class) > > > > > > > > .toRule(); > > > > > > > > > > > > > > > > /** Creates a CassandraFilterRule. */ > > > > > > > > protected CassandraFilterRule(Config config) { > > > > > > > > super(config); > > > > > > > > } > > > > > > > > > > > > > > > > /** Rule configuration. */ > > > > > > > > public interface Config extends RelOptNewRule.Config { > > > > > > > > @Override default CassandraFilterRule toRule() { > > > > > > > > return new CassandraFilterRule(this); > > > > > > > > } > > > > > > > > } > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > Intuitively, if we want to check what does the rule generally > > > > match or how > > > > > > > > it is defined, we just check the constructor. Now we > checkout the > > > > > > > > constructor, only config is there, go to Config, there is > still > > > > nothing > > > > > > > > interesting, we have to go to the INSTANCE. What is the > difference > > > > with > > > > > > > > just using operand and optionConfig as the rule constructor? > > > > > > > > > > > > > > > > protected CassandraFilterRule(RelOptRuleOperand operand, > Config > > > > config) > > > > > > > > { > > > > > > > > super(operand, config); > > > > > > > > } > > > > > > > > > > > > > > > > Or even simply replace Config with int, with each bit > represent an > > > > option, > > > > > > > > if all of them are boolean options. > > > > > > > > > > > > > > > > Nothing is more flexible than just using RelOptRuleOperand > as the > > > > > > > > parameter, just like the base class RelOptRule does. But do > we > > > > want it? > > > > > > > > > > > > > > > > At the same time, with the new approach, it is now legit to > create > > > > the > > > > > > > > following instance: > > > > > > > > > > > > > > > > private static final CassandraFilterRule INSTANCE2 = > > > > > > > > Config.EMPTY > > > > > > > > .withOperandSupplier(b0 -> > > > > > > > > b0.operand(LogicalProject.class) // Even > the is > > > > intended > > > > > > > > to match Filter, but it compiles > > > > > > > > .oneInput(b1 -> > > > > b1.operand(CassandraTableScan.class) > > > > > > > > .noInputs())) > > > > > > > > .as(Config.class) > > > > > > > > .toRule(); > > > > > > > > > > > > > > > > Before we continue to the discussion and code review, we > need to > > > > go back > > > > > > > > to the original problem, is it a real problem that is facing > us? > > > > Is there > > > > > > > > any real demand or just artificial demand? How about we > conduct a > > > > Calcite > > > > > > > > user survey to see how Calcite devs and users think? Then > let's > > > > see how to > > > > > > > > move forward. > > > > > > > > > > > > > > > > [+1] Hell yeah, the new approach is awesome, let's go with > it. > > > > > > > > [+0] Meh, I am OK with current approach, I don't see any > burden or > > > > problem > > > > > > > > with it. > > > > > > > > [-1] Hell no, current approach is bad, the new one is even > worse. > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Haisheng > > > > > > > > > > > > > > > > > > > > > > > > On 2020/06/12 23:51:56, Julian Hyde <[email protected]> > wrote: > > > > > > > > > There is now a PR: > https://github.com/apache/calcite/pull/2024. > > > > Can > > > > > > > > > people please review? > > > > > > > > > > > > > > > > > > Here's the TL;DR: > > > > > > > > > > > > > > > > > > Previously, it was not easy to customize, re-use or extend > > > > planner > > > > > > > > > rules. If you wished to customize a rule (i.e. create a new > > > > instance > > > > > > > > > of a rule with different properties) you would have to > call the > > > > rule's > > > > > > > > > constructor. Frequently the required constructor did not > exist, > > > > so we > > > > > > > > > would have to add a new constructor and deprecate the old > one. > > > > > > > > > > > > > > > > > > After this change, you start off from an instance of the > rule, > > > > modify > > > > > > > > > its configuration, and call toRule() on the configuration. > (Rule > > > > > > > > > constructors are now private, because only the > configuration ever > > > > > > > > > calls them.) > > > > > > > > > > > > > > > > > > A good illustration of this is DruidRules, which used to > contain > > > > many > > > > > > > > > sub-classes. Those sub-classes are no longer needed. Old > code: > > > > > > > > > > > > > > > > > > public static final DruidSortProjectTransposeRule > > > > > > > > SORT_PROJECT_TRANSPOSE = > > > > > > > > > new > > > > DruidSortProjectTransposeRule(RelFactories.LOGICAL_BUILDER); > > > > > > > > > > > > > > > > > > public static class DruidSortProjectTransposeRule > > > > > > > > > extends SortProjectTransposeRule { > > > > > > > > > public > DruidSortProjectTransposeRule(RelBuilderFactory > > > > > > > > > relBuilderFactory) { > > > > > > > > > super( > > > > > > > > > operand(Sort.class, > > > > > > > > > operand(Project.class, > operand(DruidQuery.class, > > > > > > > > none()))), > > > > > > > > > relBuilderFactory, null); > > > > > > > > > } > > > > > > > > > } > > > > > > > > > > > > > > > > > > New code: > > > > > > > > > > > > > > > > > > public static final SortProjectTransposeRule > > > > SORT_PROJECT_TRANSPOSE = > > > > > > > > > SortProjectTransposeRule.INSTANCE.config > > > > > > > > > .withOperandFor(Sort.class, Project.class, > > > > DruidQuery.class) > > > > > > > > > .toRule(); > > > > > > > > > > > > > > > > > > The change maintains backwards compatibility to a large > degree. > > > > In a > > > > > > > > > few places, I had to change rule instances from type > RelOptRule > > > > to > > > > > > > > > Supplier<RelOptRule>, to avoid deadlocks during class > loading. > > > > For > > > > > > > > > instance, instead of writing FilterJoinRule.FILTER_ON_JOIN > you > > > > must > > > > > > > > > now write FilterJoinRule.FILTER_ON_JOIN.get(). > > > > > > > > > > > > > > > > > > Julian > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Apr 16, 2020 at 12:08 PM Julian Hyde < > [email protected]> > > > > wrote: > > > > > > > > > > > > > > > > > > > > I have now pushed a dev branch with a prototype. Please > see > > > > > > > > > > https://issues.apache.org/jira/browse/CALCITE-3923 for > > > > details. > > > > > > > > > > > > > > > > > > > > Having built the prototype, I believe that this change is > > > > beneficial > > > > > > > > > > and we should do it. But I would like to get to > consensus on > > > > the > > > > > > > > > > design before we pull the trigger. > > > > > > > > > > > > > > > > > > > > Julian > > > > > > > > > > > > > > > > > > > > On Tue, Apr 14, 2020 at 2:06 PM Julian Hyde < > [email protected]> > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > 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 < > > > > > > > > [email protected]> 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<[email protected]> > > > > > > > > > > > > 日 期：2020年03月14日 22:54:04 > > > > > > > > > > > > 收件人：<[email protected]> > > > > > > > > > > > > 主 题：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 < > > > > [email protected]> > > > > > > > > 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 > > > > > > > > > > > > > [email protected] > > > > > > > > > > > > > > > > > > > > > > > > > > Le jeu. 20 févr. 2020 à 13:11, Julian Hyde < > > > > [email protected]> 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
