I don't have a problem with this change. I do have a problem with changes, such as the one that Ruben recently committed [1] to make the constructor of EnumerableMergeJoin, that increase the surface of our public API. The larger our public API, the more work it is to add features to Calcite while adhering to semantic versioning.
'final' is sometimes used intentionally to limit the API surface area. Julian [1] https://github.com/apache/calcite/commit/6bb7e2d0b65ec3c7a0d82c92ca0564f8caec4af5 On Thu, Jul 16, 2020 at 10:33 AM Rui Wang <[email protected]> wrote: > > +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 > > > > >
