You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/12/09 23:16:53 UTC
[GitHub] [calcite] jacques-n opened a new pull request #2639: [CALCITE-4929] Implement default getDef() methods in MetadataHandler interfaces
jacques-n opened a new pull request #2639:
URL: https://github.com/apache/calcite/pull/2639
- Add default getDef() methods.
- Mark all function metadata handler interfaces as FunctionalInterface where possible.
- Update MetdataDef and RelMetdataHandlerGeneratorUtil to ignore getDef() methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jamesstarr commented on pull request #2639: [CALCITE-4929] Implement default getDef() methods in MetadataHandler interfaces
Posted by GitBox <gi...@apache.org>.
jamesstarr commented on pull request #2639:
URL: https://github.com/apache/calcite/pull/2639#issuecomment-991220351
>
@jacques-n, it is used in ReflectiveRelMetadataProvider to support deprecated behavior, so no refactoring or minimal refactoring is required. You could just have it throw an exception since the value is never actually needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on pull request #2639: [CALCITE-4929] Implement default getDef() methods in MetadataHandler interfaces
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #2639:
URL: https://github.com/apache/calcite/pull/2639#issuecomment-991311116
> You could just have it throw an exception since the value is never actually needed.
That would be a breaking change (downstream projects may be using `getDef()`). We should deprecate for at least one release before removing and/or throwing. This means for at least one release (possibly many), the PR alleviates downstream boilerplate implementations and allows most handlers to be functional. It also allows implementers to be less impacted once the method deprecates and then is removed.
While getDef() deprecation may make sense ultimately, I'd like to see any patch that proposes that to also modify `ReflectiveRelMetadataProvider` to avoid it's use in the non-deprecated path of `public static <M extends Metadata> RelMetadataProvider reflectiveSource(
MetadataHandler<? extends M>, Class<? extends MetadataHandler<M>>)`. This seems beyond the scope of this change. Happy to review that patch separately if you'd like to propose it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jamesstarr commented on pull request #2639: [CALCITE-4929] Implement default getDef() methods in MetadataHandler interfaces
Posted by GitBox <gi...@apache.org>.
jamesstarr commented on pull request #2639:
URL: https://github.com/apache/calcite/pull/2639#issuecomment-991220351
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jamesstarr commented on pull request #2639: [CALCITE-4929] Implement default getDef() methods in MetadataHandler interfaces
Posted by GitBox <gi...@apache.org>.
jamesstarr commented on pull request #2639:
URL: https://github.com/apache/calcite/pull/2639#issuecomment-991484772
All of calcites instances would still return value, and previously all of
their instances would have had to have an implementation. You can not
break down stream code by adding a default implementation, after wall it
was one of the design goals of java 8.
On Fri, Dec 10, 2021 at 1:35 PM Jacques Nadeau ***@***.***>
wrote:
> You could just have it throw an exception since the value is never
> actually needed.
>
> That would be a breaking change (downstream projects may be using getDef()).
> We should deprecate for at least one release before removing and/or
> throwing. This means for at least one release (possibly many), the PR
> alleviates downstream boilerplate implementations and allows most handlers
> to be functional. It also allows implementers to be less impacted once the
> method deprecates and then is removed.
>
> While getDef() deprecation may make sense ultimately, I'd like to see any
> patch that proposes that to also modify ReflectiveRelMetadataProvider to
> avoid it's use in the non-deprecated path of public static <M extends
> Metadata> RelMetadataProvider reflectiveSource( MetadataHandler<? extends
> M>, Class<? extends MetadataHandler<M>>). This seems beyond the scope of
> this change. Happy to review that patch separately if you'd like to propose
> it.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/calcite/pull/2639#issuecomment-991311116>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAMO4UPALVTQSJBXA3MBSBLUQJXCLANCNFSM5JXXFVVA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on pull request #2639: [CALCITE-4929] Implement default getDef() methods in MetadataHandler interfaces
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #2639:
URL: https://github.com/apache/calcite/pull/2639#issuecomment-991139051
> Have you consider adding a default implementation directly to handler interface and deprecating the method?
It seems like your saying to add a default implementation to `MetadataHandler.getDef()`. I considered this but it requires the use of reflection to retrieve an expected static field on sub-interfaces. To me, this perpetuates a kind of brittle runtime binding that I think we should avoid/reduce in Calcite.
I didn't consider deprecating the method as I believe it is used in ReflectiveRelMetadataProvider and it seems like it would need to be refactored to support removal. My main goal was that people implementing handlers didn't have to build a bunch of boilerplate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on pull request #2639: [CALCITE-4929] Implement default getDef() methods in MetadataHandler interfaces
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #2639:
URL: https://github.com/apache/calcite/pull/2639#issuecomment-991311116
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n commented on pull request #2639: [CALCITE-4929] Implement default getDef() methods in MetadataHandler interfaces
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #2639:
URL: https://github.com/apache/calcite/pull/2639#issuecomment-991723967
@jamesstarr, you're right that technically it wouldn't be a breaking change. However, it doesn't seem like a good approach since it is a misleading change. Having getDef() throw and marking the subinterfaces as functional means you have a suggested interface that doesn't actually work in the reflective provider. The change here avoids causing that unexpected/misleading behavior. As I said, if you want to get rid of the method moving forward, I'm fully supportive of doing that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] jacques-n merged pull request #2639: [CALCITE-4929] Implement default getDef() methods in MetadataHandler interfaces
Posted by GitBox <gi...@apache.org>.
jacques-n merged pull request #2639:
URL: https://github.com/apache/calcite/pull/2639
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org