None of your 4 points prove that anything is fundamentally broken. In particular, the ability to represent cyclic graphs is a feature, not a bug.
It’s not “too late” if a rule fires and creates a RelNode and that RelNode’s constructor asserts. Because that rule is being run in a debugger, as part of a developer’s test, and the developer can just re-run the test. The real question is: how do we reduce the amount of time and effort for the developer to figure out what is wrong and fix the problem? I claim that an assert in a constructor is simple and direct. Julian > On Feb 3, 2019, at 12:47 PM, Vladimir Sitnikov <[email protected]> > wrote: > > 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
