[
https://issues.apache.org/jira/browse/CALCITE-2435?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16565850#comment-16565850
]
Vladimir Sitnikov commented on CALCITE-2435:
--------------------------------------------
{quote} What happened to DelegatingTestFactory?{quote}
It [sank|https://www.youtube.com/watch?v=2RAzI7UEkhE]. Well, I believe it makes
no sense to have DelegatingTestFactory as it results in awkward and error-prone
API like {{factory.createValidator(factory)}}. It is very hard to reason on
"which properties will eventually be used", and I was not sure how could I pass
"Parser.Config" to the SqlValidator instance.
The refactoring makes the code much simpler (just one SqlTestFactory class
instead of previous SqlTestFactory
+DefaultSqlTestFactory+DelegatingSqlTestFactory), and it is way easier to
follow (there's no {{factory.createAbc(factory)}} anymore).
The upcoming "add parserConfig to SqlAdvisor" change would be a trivial change
to SqlTestFactory:
{code:java} public SqlAdvisor createAdvisor() {
SqlValidator validator = getValidator();
if (validator instanceof SqlValidatorWithHints) {
- return new SqlAdvisor((SqlValidatorWithHints) validator);
+ return new SqlAdvisor((SqlValidatorWithHints) validator, parserConfig);
// <-- just a single line change, no delegators involved
}
throw new UnsupportedOperationException(
"Validator should implement SqlValidatorWithHints, actual validator is
" + validator);
}{code}
{quote} What happened to the cache{quote}
Technically speaking, the cache (for MockCatalogReader instances) works since
most of SqlTestFactory instances are cached to the static final fields.
On top of that, there's cache right in MockCatalogReader.create, however it is
a bit hard to do right since JavaTypeFactory is mutable, and MockCatalogReader
depends on TypeFactory.
That is we should either reuse TypeFactory across test instances, or we cannot
properly cache the catalog.
I have measured the number of times {{MockCatalogReader}} is initialized, and
it is not that many. The key problem is JavaTypeFactory.
We might want to rewrite the thing with a help of DI (e.g. Guice) to
instantiate objects in a proper order, however I find current approach (manual
creation of the relevant objects in SqlTestFactory constructor) to be good
enough.
{quote}Please don't put ');' on its own line. That's not the house style.{quote}
Oh, indeed.
> Refactor SqlTestFactory
> -----------------------
>
> Key: CALCITE-2435
> URL: https://issues.apache.org/jira/browse/CALCITE-2435
> Project: Calcite
> Issue Type: Bug
> Components: core
> Affects Versions: 1.17.0
> Reporter: Vladimir Sitnikov
> Assignee: Julian Hyde
> Priority: Major
>
> Current SqlTestFactory/DelegatingSqlTestFactory is hard to reason about, and
> it is hard to extend/cache properly.
> There are three key changes here:
> 1. Rename DefaultSqlTestFactory to SqlTestFactory. SqlTestFactory was an
> interface, however it has a single viable implementation, so it makes little
> sense to keep it as is.
> 2. Move MockCatalogReader.init and MockCatalogReader.init2 to
> MockCatalogReaderSimple and MockCatalogReaderExtended
> 3. Move MockCatalogReader to its own package, and split several inner classes
> to a top-level ones.
> 4. Replace usages
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
