hsyuan commented on a change in pull request #1130: [CALCITE-2865]
FilterProjectTransposeRule generates wrong traitSet when copyFilter/Project is
true
URL: https://github.com/apache/calcite/pull/1130#discussion_r272755731
##########
File path:
core/src/main/java/org/apache/calcite/rel/rules/FilterProjectTransposeRule.java
##########
@@ -155,8 +158,12 @@ public void onMatch(RelOptRuleCall call) {
final RelBuilder relBuilder = call.builder();
RelNode newFilterRel;
if (copyFilter) {
- newFilterRel = filter.copy(filter.getTraitSet(), project.getInput(),
- simplifyFilterCondition(newCondition, call));
+ final RelNode input = project.getInput();
+ final RelTraitSet traitSet = filter.getTraitSet()
+ .replaceIfs(RelCollationTraitDef.INSTANCE,
Review comment:
But with the default INSTANCE, the RelBuilder comes from logical (or some
other convention) RelBuilder factory, so using a logical builder to build a
physical (another convention) operator still doesn't seem right. I don't think
we can cater for everyone with a single default rule INSTANCE, if the default
one doesn't satisfy his/her needs, he/she should create a new one to match
operator with expected convention and relbuilder.
Even for those who don't want to sweat on building rule sets or multi-phase
planning, I still don't see why the rule should match both logical and physical
operators. Say we have
1.
```
LogicalFilter
+-- LogicalProject
```
, with transpose rule, we will have
2.
```
LogicalProject
+-- LogicalFilter
```
, with implementation rules, we will have
3.
```
EnumerableFilter
+-- EnumerableProject
```
and
4.
```
EnumerableProject
+-- EnumerableFilter
```
Again, with transpose rules (it can match physical operators), it matches
expression 3, creates new relational expression like 4, and matches 4, creates
relational expression same with 3. It is not only redundant, but will also
increase the optimization time.
This is also why in our system, almost all the default Calcite rule
INSTANCEs can't be used, we have to create new ones.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
