+1 to everything here.
* Thanks to James for starting with a design discussion rather than a PR
* And also thanks for starting it on the mailing list (where the most
people will see it)
* As Jacques says, now is a good time to move the discussion to a Jira case
* Thank you both for keeping up the push to modernize this important
but challenging component while keeping necessary compatibility
On Wed, Dec 22, 2021 at 10:25 AM Jacques Nadeau <jacq...@apache.org> wrote:
>
> Hi James, I think it is great that you are pushing this effort forward. I
> have a few requests:
>
> - Can we have detailed discussions around code, etc on Jira instead of the
> ml? It's really hard to read here and also tends to get lost over time.
> - If we think we need to make one or more breaking changes, I hope we can
> minimize the number of times we need to make a breaking change. For
> example, If we change the generic signature of MetadataHandler, there are
> probably some other changes we should discuss before finalizing the
> specific breaking change so we don't break it twice.
> - I'm really interested in this issue but I'm also planning on spending
> most of the next 5 days or so with family due to our holiday traditions.
> Can we iterate on this set of things next week?
>
>
>
> On Tue, Dec 21, 2021 at 4:38 PM James Starr <jamesst...@gmail.com> wrote:
>
> > While learning calcite, then during subsequent discussions, the legacy code
> > in the metadata sub-system caused significant understanding/discussion.
> >
> > Currently to define a rel metadata type:
> >
> > public class MyMetadata extends Metadata {
> > MetadataDef DEF = ...
> >
> > VALUE myMethod1();
> >
> > class Handler extends RelHandler<MyMetadata> {
> > VALUE myMethod2(RelNode, MetadataQuery)
> > }
> > }
> >
> > However, it could be reduced to:
> >
> > class MyMetadataHandler extends RelHandler {
> > VALUE myMethod(RelNode, MetadataQuery)
> > }
> >
> > In BuiltInMetadata the resulting definitions would look like:
> >
> > @Deprecated // to be removed before 2.0
> >
> > public class MyMetadata extends Metadata {
> >
> > MetadataDef DEF = ...
> >
> > VALUE myMethod1();
> >
> >
> > @FunctionalInterface
> > @Deprecated // to be removed before 2.0
> >
> > class Handler extends MyMetadataHandler {
> >
> > @Deprecated // to be removed before 2.0
> >
> > @Override default MetadataDef<MyMetadata> getDef() {
> >
> > return DEF;
> >
> > }
> >
> > }
> > }
> >
> >
> > @FunctionalInterface
> >
> > class MyMetadataHandler extends RelHandler {
> > VALUE myMethod(RelNode, MetadataQuery)
> > }
> >
> >
> > This would minimize breaks to downstream projects since reference to
> > existing handlers would not be broken and downstream projects could migrate
> > at their own leisure.
> >
> > I would like to remove the generic from MetadataHandler as part of cleaning
> > up the boilerplate for defining Rel Metadata types[CALCITE-4942].
> > Currently, it refers to the sister Metadata class that is no longer used in
> > a non-deprecated code. Furthermore, it does not not provide type safety to
> > any code, as all usages of it are either <?> or <? extends Metadata>. This
> > would be a breaking change for downstream projects that have a custom
> > metadata type or have their own implementation of MetadataHandlerProvider.
> > Most downstream projects have neither of them, and if they do, removing the
> > generic should be straight forward. Alternatively, it could be left as is
> > and/or the "extends Metadata" constraint could be removed.
> >
> > As part of this, I could also split all the Handlers into functional
> > interfaces, which would help with Jacques work to not use Janino.
> >
> > My Draft request: https://github.com/apache/calcite/pull/2655
> >
> > James
> >
