+1 to make getRelTypeName overridable. Not a java language expert, to me when there are cases that a public API with final cannot solve, it's a good sign to remove that final to allow customization to solve those specific use cases.
-Rui On Thu, Jul 16, 2020 at 8:58 AM Haisheng Yuan <[email protected]> wrote: > +1 to remove final. > > The method returns the logical/physical operator name. When we explain the > plan (and compute the digest in 1.23.0 and before), getRelTypeName may be > called multiple times for a single operator, every time it will start from > scratch to figure out what is correct operator name. Making it overridable > will not only solve the case Ruben mentioned below, but also remove the > duplicate computation by just returning the literal string for the operator > name, even if the gain might be negligible. > > The downside is when downstream projects who override the method refactor > the operator class name, they have to manually update the method to return > correct names. We get a flexibility by losing another one. But I think it > is OK to add a caveat that use at your own risk. > > On 2020/07/16 14:32:31, Ruben Q L <[email protected]> wrote: > > Hello everyone, > > > > I have a small issue regarding RelNode#getRelTypeName [1]. This method is > > used when explaining a plan (e.g. via RelOptUtil#dumpPlan), and it > "returns > > the name of this relational expression's class, sans package name, for > use > > in explain". > > This method has a *final* implementation in AbstractRelNode [2], which > > returns a String based on getClass().getName(). Normally, this > > implementation should work fine for all RelNode implementations, but > > currently I am working on a project that uses obfuscation, so this > > implementation returns a non-meaningful name on our RelNode > > implementations (e.g. our TableScan operators), which will lead to > > incomprehensible query plans in our logs. I would like to override the > > method on our RelNode implementations, so that they can return a "clear" > > relTypeName, instead of the default one based on getClass().getName(), > but > > at the moment I cannot do that, since the implementation in > AbstractRelNode > > is final. > > > > What would you think about removing the "final" from > > AbstractRelNode#getRelTypeName and allow downstream projects to provide > > their own implementation? Of course, these projects shall be responsible > > for providing a valid implementation at their own risk, we could even > add a > > warning message in AbstractRelNode#getRelTypeName saying something like > "do > > not override this method unless you know what you are doing" or "it is > > strongly discouraged to override this method". > > > > Of course, I know there are other alternatives for my problem, like > > implementing my own dumpPlan and RelWriter mechanisms, but I would like > to > > explore the getRelTypeName possibility before doing that, because it > would > > be a more straightforward solution, and it might be useful for other > people > > as well. > > > > Thanks and best regards, > > Ruben > > > > [1] > > > https://github.com/apache/calcite/blob/551e3f5a182a48e20586e40c378224bb63e4bfb3/core/src/main/java/org/apache/calcite/rel/RelNode.java#L367 > > [2] > > > https://github.com/apache/calcite/blob/551e3f5a182a48e20586e40c378224bb63e4bfb3/core/src/main/java/org/apache/calcite/rel/AbstractRelNode.java#L182 > > >
