[
https://issues.apache.org/jira/browse/CALCITE-2970?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17076893#comment-17076893
]
Xiening Dai commented on CALCITE-2970:
--------------------------------------
Thanks [~danny0405] for the feedback. My comments inline -
{quote}
Modify the RelBuilder directly makes the RelBuilder too complex to use, i still
think the builder only needs to build logical nodes, the node with a convention
is an implementation thing and it's the duty of AbstractRelOptPlanner to make
the desired convention tree.
{quote}
What do you mean by too complex to use? The interface remains the same. The
only difference is the new method withConvention() which allows user to provide
a convention when building a particular relnode. It is also optional, meaning
that the user can ignore it and create logical nodes in the exact same way as
they did before.
{quote}
With your change, we need to register a factory for specific convention node,
while what we do inside is not "building a rel node" but "change a node
convention", the introduced mechanism is too heavy and it happens where it
shouldn't.
{quote}
The syntax is not "change a node convention". The syntax is "build a rel node,
optionally with X convention". To register this mechanism, you would need a
concrete rel node factory for particular convention. It usually only involves
one line of code.
{quote}
The RelBuilderFactory changes from an intertface to impl class so that we can
not use it as a lambda which is a breaking change.
{quote}
Any specific concern regarding this change? All the API and class definitions
are not set in stone. We care about backward compatibility but at the same time
we should be able to evolve when appropriate.
{quote}
Compared to RelFactories.LOGICAL_BUILDER, RelFactories.DEFAULT_BUILDER is a bad
name, the keyword `default` means nothing.
{quote}
'Default' means a pre-selected option. We have such naming all over the places
- DEFAULT_FILTER_FACTORY, DEFAULT_PROJECT_FACTORY, etc. If you have better
name, I am all ears. Thanks.
> Performance issue when enabling abstract converter for EnumerableConvention
> ---------------------------------------------------------------------------
>
> Key: CALCITE-2970
> URL: https://issues.apache.org/jira/browse/CALCITE-2970
> Project: Calcite
> Issue Type: Bug
> Components: core
> Reporter: Haisheng Yuan
> Priority: Major
> Labels: pull-request-available
> Time Spent: 11h 20m
> Remaining Estimate: 0h
>
> If we enable the use of abstract converter for {{EnumerableConvention}}, by
> making {{useAbstractConvertersForConversion}} return true,
> {{JDBCTest.testJoinManyWay}} will not complete.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
