[
https://issues.apache.org/jira/browse/CALCITE-4963?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17464844#comment-17464844
]
Marco Jorge edited comment on CALCITE-4963 at 12/23/21, 11:39 PM:
------------------------------------------------------------------
Thanks for the feedback [~julianhyde] .
We could just push the "{{{}DatabaseMetaData to SqlDialect.Context{}}}" into a
static method (in the PR I pushed it to
{{{}SqlDialectFactoryImpl.Utils#context{}}}) - if you would prefer somewhere
else let me know.
In order to reuse the default selection method from {{SqlDialectFactoryImpl}}
in a custom factory it seems however unnecessary that the
{{SqlDialectFactoryImpl}} repeats the same "{{{}DatabaseMetaData to
{{SqlDialect.Context}}{}}}" conversion. With the static method I proposed and
the 2 entry points for the {{#create}} we can easily avoid inefficient calls.
Please note that I did not change the interface for {{SqlDialectFactory}} and
hence I didn't make it any more complicated than it was before for someone to
create their own implementation.
I can live with just the static method for "{{{}DatabaseMetaData to
SqlDialect.Context{}}}" if you feel strongly about the {{#create}} - just let
me know and I'll update the PR. Also, I'll make sure to add a test as per your
request.
As for the chain of responsibility, for my particular challenge it is not
necessary, but I can see how the whole {{SqlDialectFactoryImpl}} could benefit
from such a refactoring instead of the current if-then-else/switch approach,
especially together with a service provider pattern and then moving all those
particular dialects out of calcite-core and into some calcite-dialects module.
was (Author: marcobjorge):
Thanks for the feedback [~julianhyde] .
We could just push the "{{{}DatabaseMetaData to SqlDialect.Context{}}}" into a
static method (in the PR I pushed it to
{{{}SqlDialectFactoryImpl.Utils#context{}}}) - if you would prefer somewhere
else let me know.
In order to reuse the default selection method from {{SqlDialectFactoryImpl
}}in a custom factory it seems however unnecessary that the
{{SqlDialectFactoryImpl}} repeats the same "{{{}DatabaseMetaData to
SqlDialect.Context{}}}" conversion. With the static method I proposed and the 2
entry points for the {{#create}} we can easily avoid inefficient calls. Please
note that I did not change the interface for {{SqlDialectFactory}} and hence I
didn't make it any more complicated than it was before for someone to create
their own implementation.
I can live with just the static method for "{{{}DatabaseMetaData to
SqlDialect.Context{}}}" if you feel strongly about the {{#create}} - just let
me know and I'll update the PR. Also, I'll make sure to add a test as per your
request.
As for the chain of responsibility, for my particular challenge it is not
necessary, but I can see how the whole {{SqlDialectFactoryImpl}} could benefit
from such a refactoring instead of the current if-then-else/switch approach,
especially together with a service provider pattern and then moving all those
particular dialects out of calcite-core and into some calcite-dialects module.
> Extensibility of SqlDialectFactory lacks reusability of
> SqlDialectFactoryImpl.*
> -------------------------------------------------------------------------------
>
> Key: CALCITE-4963
> URL: https://issues.apache.org/jira/browse/CALCITE-4963
> Project: Calcite
> Issue Type: Improvement
> Components: core
> Reporter: Marco Jorge
> Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> _(nicetities first - great project and thanks all for the great work)_
> Although it's possible to extend the SqlDialectFactory, the custom
> implementation cannot reuse any of the behaviour of the default
> SqlDialectFactoryImpl. The default SqlDialectFactoryImpl has lots of
> important/reusable behaviour such as #getCasing, #isCaseSensitive,
> #getNullCollation or even the default decisioning in the #create method.
> If any user needs to provide a custom SqlDialect yet still support the
> existing SqlDialects the user needs to copy the whole SqlDialectFactoryImpl
> to make the custom changes.
> This request is to make the default behavior in SqlDialectFactoryImpl
> reusable so that extensions don't need to fork a whole class.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
