[
https://issues.apache.org/jira/browse/CALCITE-4544?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17396242#comment-17396242
]
Julian Hyde commented on CALCITE-4544:
--------------------------------------
At a high level, it looks good to me. We should do this. Your code is clean and
well-structured.
At a low level, I see minor nits: a javadoc comment with '..', missing space in
'.*/', a deprecation annotation without 'to be removed before ...' comment.
I am too busy for the next two weeks (and probably afterwards) to commit this.
That requires finding all of the nits, thinking about impact, testing, and many
other things. I have sent a message on dev@ appealing for another committer.
> Deprecate Metadata API backed by Java Reflection
> ------------------------------------------------
>
> Key: CALCITE-4544
> URL: https://issues.apache.org/jira/browse/CALCITE-4544
> Project: Calcite
> Issue Type: Improvement
> Reporter: James Starr
> Assignee: James Starr
> Priority: Major
> Labels: pull-request-available
> Time Spent: 40m
> Remaining Estimate: 0h
>
> A large portion of the metadata api is functionally deprecated, we should
> make it explicit.
> Apache Calcite currently has 2 ways deriving metadata about rel node, one
> through runtime generated handler classes that are compiled using Janino, and
> the other using java reflection. The former will be referred to as Janino
> and the latter as java reflection for brevity. Java reflection is not used
> due to performance reason in calcite main, nor is it tested, and using it can
> lead to degraded performance due to interactions with caching of the janino
> handlers. Both share a common way of registering classes with implementation
> by using ReflectiveRelMetadataProvider and ChainedRelMetadataProvider. There
> are also 2 different subsystems for supporting metadata caching, one where a
> reference to a cache is stored in the metadata query and the other where
> caching is a wrapper pattern around the providers. There are also 2 separate
> ways for invalidating the caching, one with a counter stored in the planner
> used by java reflection api, and another where volcano planner tracks the
> parents of all rel nodes that it mutates and then explicitly invalidates
> those parents used by the janino api.
> So the following could be removed without the lose of any functionality:
> * CachingRelMetadataProvider - used for java reflection base metadata caching
> * RelOptPlanner.getRelMetadataTimestamp - used by java reflection to
> invalidate the metadata cache
> * RelOptPlanner.registerMetadataProviders(List<RelMetadataProvider> list) -
> used by java reflection API to support of Hep and Volcano nodes.
> * MetadataFactory - used to caching an internal artifact of the
> implementation of java reflection API
> * HepRelMetadataProvider - used by java reflection to support Hep node
> delegation to a child node.
> * VolcanoRelMetadataProvider - used by java reflection to support RelSubset
> node delegation to a child node.
> * AbstractRelNode.metadata - i think was an even older api that now bridges
> to the java reflection api
> * RelNode.metadata - see above
> * MetadataDef.metadtaClass - used by java reflection and for a unique
> descriptive name of the janino generated handler. Janino dependency on it is
> easily broken.
> * ReflectiveRelMetadataProvider. map and metadataClass0 - used by java
> reflection implementation
> * RelMetadataProvider.apply() - most of the actual implementation java
> reflection API
> Metadata and MetadataDef could also be removed, however removing it would
> require reworking portions of janino code do so.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
