If we focus on the initial problem "rules fires for a single convention only" I think your idea of enforcing this using RelOptRule#matches is the cleanest and least intrusive. The default implementation instead of returning true it could check that all operands are in the same convention. Specialized rules could define an alternative behavior. I am not sure however how it will turn out in terms of efficiency.
Στις Τρί, 29 Ιαν 2019 στις 7:32 π.μ., ο/η Vladimir Sitnikov < [email protected]> έγραψε: > Julian>In VolcanoPlanner, DrillUnion would not be an input to JdbcFilter > (it might be in a different RelSubset of the same RelSet, but not the same > RelSubset), and therefore I don’t think the rule would fire. > > That depends on whoever creates the relations. Technically speaking, there > might be a valid reason to create DrillUnion over JdbcFilter. > I would agree it is not something that is expected to happen often, but it > it can happen. > On top of that, org.apache.calcite.prepare.RelOptTableImpl#toRel easily > creates EnumerableTableScan out of thin air, so it is another source of > cross-convention rels > > Of course it can happen due to bug in a rule. > For instance, ProjectJoinTransposeRule is declared as > operand(Project.class, operand(Join.class, any())), and it would easily > match DrilProject(DrillJoin(...)), then replace it with Logical rels. > > How do you suggest to fix that? > > Certain Calcite rules use "(Class<? extends Join> clazz, RelBuilderFactory > relBuilderFactory)" constructor pattern, however that does not play nicely > with SetOps. > Which "clazz" should you pass in order to match "DrillSetOps only"? > On top of that, it requires to write classes again and again while > providing no type safety. > > Julian> Also, I try not to give Convention special treatment that is not > given to other traits. I wonder whether this could be generalized to other > traits? Say, that a rule matches only if the matched RelNodes are have the > same trait. > > I see no way to generalize that in a usable way. > > We could do better error reporting by verifying traits at call#transformTo > time. > In other words, most (all?) of the time we expect that traits of the source > node and the traits of the equivalent one should somehow match. > However it is still required to efficiently pick the proper operands before > rule starts. > > > RelBuilderFactory (and RelBuilder) is concerned with creating RelNodes > not matching rules, so doesn’t seem the right place for the getConvention() > method. > > In fact, it could even be used for assertions that the produced nodes match > *intended* convention. Currently the intended convention is declared in > comments at best (see > > https://github.com/apache/drill/blob/b67f77a18ac4b67044fa9b4b962d0536fdc54af5/exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRelFactories.java#L77 > ) . > > > Vladimir >
