Thanks for you input. I think this doesn’t relates to the Volcano theory. I don’t try to separate the logical transformation from physical implementation. What I propose is not to fire the same transformation rule on physical nodes as those transformation are already done on the logic nodes.
I tested out my patch, and all UTs passed. I can open a PR for that. > On Oct 28, 2019, at 2:18 PM, Stamatis Zampetakis <zabe...@gmail.com> wrote: > > Hello, > > It is not surprising that by reducing the scope of the rules the > planning times goes down since we are generating less alternatives. > > Mixing logical with physical optimizations is costly but it is one of the > big benefits offered by the Volcano framework in contrast with the System-R > style optimization where logical and physical transformations are > completely separated. > > Indeed in some cases having the same rule fire for both matching logical > and physical nodes does not make sense so we could apply some tunings. > > @Xiening: Have you tried running all tests with the modifications you > mentioned? Aren't we missing any good plans? > @Haisheng: What do you mean by saying that rules should match logical > operators only? > > Best, > Stamatis > > On Mon, Oct 28, 2019 at 8:54 PM Haisheng Yuan <h.y...@alibaba-inc.com> > wrote: > >> Agree, it is indeed redundant. We had a discussion about this in pull >> request #1130 [1]. >> >> Many of these rules not only match physical operaors, but still generate >> new logical operators. >> >> IMHO, rules should match logical operators only. >> >> [1] https://github.com/apache/calcite/pull/1130 >> >> On 2019/10/28 16:47:55, Xiening Dai wrote: >>> Hi all, >>> >>> While I was looking at CALCITE-2970, I noticed that some of the rules >> are fired for both logical and physical nodes. For example, >> ProjectMergeRule matches Project.class, so it’s fired for LogicalProject. >> But then after LogicalProject is converted into EnummerableProject, the >> same rule is fired again for the physical rels. Same for >> EnumerableLimitRule, SortRemoveConstantKeysRule, etc. >>> >>> This seems to be unnecessary. When ProjectMerge is applied to >> LogicalProject nodes, we already generate all possible alternatives with >> merged projects. We just need to convert the LogicalProject into >> EnumerableProject. There’s no need to merge EnumerableProject again. >>> >>> If I update those rules to only match logical nodes, the planning time >> of the case in CALCITE-2970 is reduced ~30%. >>> >>> Any thoughts? >> >>
