Several of the constructors of Enumerable RelNodes have the comment "Use {@link
#create} unless you know what you're doing”. I think it’s pretty clear - the
class and constructor had to be public for various reasons, but people should
not treat it as a blank check.
If your main motivation is
Agree with Ruben.
I don't see any convincing reasons to make EnumerableMergeJoin special.
On 2020/07/17 07:46:35, Ruben Q L wrote:
> Regarding CALCITE-4123, I can provide a bit more of context.
>
> I have been working on some improvements in EnumerableMergeJoin (support
> new join types). I
Regarding CALCITE-4123, I can provide a bit more of context.
I have been working on some improvements in EnumerableMergeJoin (support
new join types). I found that the easiest way to do so was extending the
class as MyOwnEnumerableMergeJoin and override the "implement" method, so
that instead of
+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
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
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
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
+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 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
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
10 matches
Mail list logo
