If I am right, I am the release manager of 1.24.
I suggest releasing 1.24 a bit later(at the end of this month)
since 1.23 was released not long ago(on 2020.5.23).
Best,
Chunwei
On Fri, Jul 3, 2020 at 10:54 PM Michael Mior <mm...@apache.org> wrote:
> I'll just add for the relatively common case where a rule is matching
> a node with a specific node as a child, and so on that we could easily
> add a less verbose API to make this part of the rule definition
> equally concise.
>
> --
> Michael Mior
> mm...@apache.org
>
> Le dim. 14 juin 2020 à 15:36, Haisheng Yuan <hy...@apache.org> a écrit :
> >
> > 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 <jh...@apache.org> 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 <jh...@apache.org> 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 <jh...@apache.org>
> 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 <
> 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
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > >
>
