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
