Still the primary goal of RelBuilderFactory is to create a RelBuilder. If getConvetion does not have an impact on the created RelBuilder then this behavior will be also confusing.
I like better your suggestion to verify traits at call#transformTo. Maybe we could say that if RelOptRule#getOutTrait returns null then input traits and output traits should always match. It doesn't sound bad to allow only a Converter to modify traits. Στις Πέμ, 31 Ιαν 2019 στις 2:43 μ.μ., ο/η Vladimir Sitnikov < [email protected]> έγραψε: > TL;DR: https://github.com/apache/calcite/pull/1019 passes tests for > calcite-core, and it now fails Cassandra adapter test. > > I'm inclined to try adding Map<Class<? extends RelNode>, Convention> > kind of registry. > Then Calcite would understand than operand(EnumerableProject.class) > means ENUMERABLE convention no matter what RelBuilder is provided. > > In fact, RelBuilderFactory is often not needed, so I'm inclined to try > allow writing RelOptRules that do not use RelBuilderFactory. > In other words, if RelBuilderFactory is not passed to the super(..) > constructor, then it would require root operand to imply a convention. > > > > > Stamatis > This change would imply that the RelBuilder creates plans > that are in a > Stamatis > single convention. > > Technically speaking, it would not. > I don't suggest adding a method to RelBuilder. > > Note: currently RelBuidlerFactory tries to unwrap factories out of > Context, and it does that in a quite lenient way. > For instance, we haven't relBuilder.calc(..) yet. Would adding > relBuilder.calc(...) and a relevant default Logical calc factory count > as a backward-incompatible change? > I think it would be incompatible since it would silently produce wrong > results (i.e. produce unexpected LogicalCalc instead of MongoCalc if a > rule is used for MongoRelFactories). > > Note2: RelBuidlerFactory is used in rules 99.42% of the times, so it > makes sense to use it to signal "the intended convention for the rule > to operate on". > > Tests show that certain rules don't really use relBuilder, but they > just .copy(..) relations. For instance, > > public CalcMergeRule(RelBuilderFactory relBuilderFactory) { > super( > operand( > Calc.class, > operand(Calc.class, any())), > relBuilderFactory, null); > > The fun part is its javadoc is full of LogicalCalc, however the rule > works for all calc subtypes since the rule uses "topCalc.copy(" > approach to get the proper calc subtype. > > > In fact, RelBuilderFactory does not make sense for CalcMergeRule since > the rule is never supposed to use RelBuilderFactory. > > > How about we implement "don't care RelBuilderFactory" > > > 8><------- cut here ----- ><8 > > Re Cassandra: > > For instance: > private CassandraLimitRule() { > super(operand(EnumerableLimit.class, > operand(CassandraToEnumerableConverter.class, any())), > "CassandraLimitRule"); > > This rule uses "LOGICAL_BUILDER", however uses EnumerableLimit.class > which is "obvious" to have ENUMERABLE convention. > > Note: certain rels have asserts in the constructor like "convention is > ENUMERABLE" or "convention of the input is the same as convention of > the rel". > > If we had a registry that says "EnumerableLimit" always has ENUMERABLE > convention, then we we don't really need extra information for rule > operands. > In other words, operand(Limit.class, ...) would require > RelBuilderFactory#getConvention while operand(EnumerableLimit.class, > ...) is good enough alone. > > > Vladimir >
