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
>
