[
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:37 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 in an extension 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 in an extension 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.
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.
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.
I'll make sure to add a test as per your request.
> 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)
