Thanks for the feedback, I will create a Jira ticket to carry out the change and remove the "final" from AbstractRelNode#getRelTypeName.
@Julian, I am sorry I committed that change ( https://issues.apache.org/jira/browse/CALCITE-4123) without further discussion. I honestly thought it was a minor detail. We can discuss it, and if the community does not agree with the change, we can revert it. Basically, the package-private constructor in EnumerableMergeJoin prevents it from being extended in downstream projects. This is not consistent with other Join operators, such as EnumerableHashJoin, EnumerableNestedLoopJoin or EnumerbleBatchNestedLoopJoin, all of them providing a protected constructor. So I do not see a reason why a downstream project can extend e.g. EnumerableHashJoin, but is unable to extend EnumerableMergeJoin. I think there is no issue in extending these clases (otherwise they would be final). I believed changing the constructor from package-private to protected would solve this issue, without causing any problematic side-effect. Best, Ruben Le jeu. 16 juil. 2020 à 21:32, Julian Hyde <[email protected]> a écrit : > 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 > > > > > > > >
