I have force-pushed to https://github.com/apache/calcite/pull/2024.
There are now some commits to be merged before 1.24 (hopefully today
or tomorrow) and some to be merged after 1.24.
Can someone give it a final review?
Chunwei,
Please, let's do the 1.24 release sooner rather than later. The part
of this change that has to occur after 1.24 is large (204 files, 9k
lines added, 6k lines removed) and so is susceptible to bit-rot. I
agreed to Stamatis' proposal to do this in two phases on the
assumption that the release was not far off.
Smaller releases are easier for the release manager, too. :)
Julian
On Mon, Jul 6, 2020 at 1:40 AM Chunwei Lei <chunwei.l...@gmail.com> wrote:
>
> 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
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > >
> >
