Since the concern on CALCLITE-4123 is brought up, can we make a quick decision about whether to make it in the 1.24.0 release or not?
Having the public interface increased in 1.24.0 and then later there might be a decision to revert it doesn't sound right. If we are not sure for now, maybe have a quick revert to unblock 1.24.0 release first? -Rui On Thu, Jul 16, 2020 at 3:17 PM Ruben Q L <rube...@gmail.com> wrote: > 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 <jh...@apache.org> 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 <amaliu...@apache.org> 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 <hy...@apache.org> > 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 <rube...@gmail.com> 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 > > > > > > > > > > > >
