Julian> It seems to me that Vladimir is trying to introduce a particular kind of order because it seems “tidy”, not because anything is fundamentally broken.
Collapsing EnumerableFilter(EnumerableFilter(...)) into LogicalFilter is (fundamentally) broken: 1) It creates cycles in Volcano graph, and those cycles cause infinite planning time (see CALCITE-2223 for test cases) 2) It spends lots of time by performing irrelevant rules. As you can see, Travis time reduces from 1h34m (regular CI for now: https://travis-ci.org/apache/calcite/builds/487420311) to 1h23m (with fix: https://travis-ci.org/apache/calcite/builds/487552497 ). Individual jobs improve like 17m -> 15m. That is quite noticeable. 3) As you have said "decorrelator" is supposed to operate with LOGICAL convention only. That means toRel should try its best to represent the output in LOGICAL format. 4) org.apache.calcite.rel.RelCollationTraitDef#convert assumes it can always convert from Convention.NONE. That however is not always true. Drill runs Volcano plans without Convention.NONE rules (it does have drill_logical and drill_physical conventions/rules for that part though), so org.apache.calcite.rel.RelCollationTraitDef#convert just fails. Julian>If a rule does the wrong thing, don’t blame the engine, fix the rule. Unfortunately, lots of rules (even in Calcite codebase!) are broken. That means developers (non-Volcano experts) will fail to create a rule with probability of 99.42% Julian> Also, if a particular RelNode requires its input to be a particular trait (for example, EnumerableFilter requires its input to be Enumerable) then its constructor should assert. Assert in a constructor is a nice thing, however it is too late. We don't really want to fire a rule, perform the transformation and fail in the constructor. We do want to know if the rule is applicable beforehand. Vladimir
