+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 >
