+1 for removing the final modifier for all the reasons mentioned so far. Regarding CALCITE-4123, I find Ruben's justification convincing. The majority of Enumerable operators (not only joins) are extensible (with public/protected constructors) so it seems that EnumerableMergeJoin was the only exception to this pattern. Now if we want to keep these APIs internal it is another discussion and I think it should consider all Enumerable operators so for the moment I am fine with the current change. The only question from my side is why it is necessary to extend EnumerableMergeJoin in the first place but this is mostly from curiosity, and to see if there are things that should be changed in the operator to avoid such extensions. Let's continue the discussion under the respective JIRA if necessary.
Best, Stamatis On Fri, Jul 17, 2020 at 1:30 AM Rui Wang <amaliu...@apache.org> wrote: > 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 > > > > > > > > > > > > > > > > >
