[CALCITE-4882] Introduce new Lambda-based Metadata framework #2603
Conversation
jacques-n
commented
Nov 10, 2021
jacques-n
commented
- Introduce a Lambda-based RelMetadataQuery creator for AOT environments
- Add new microbenchmark for metadata retrieval on a five-way-join
- Fix incorrect results for disabled test JdbcTest.testJoinFiveWay()
- Update jmh-gradle-plugin version.
|
Working to update |
- Introduce a Lambda-based RelMetadataQuery creator for AOT environments - Add new microbenchmark for metadata retrieval on a five-way-join - Fix incorrect results for disabled test JdbcTest.testJoinFiveWay() - Add support for multiple RelMetadataQuery test versions. - Update jmh-gradle-plugin version.
jacques-n
force-pushed
the
CALCITE-4882
branch
from
043f24d to
b69ffa2
Compare
|
Added reuse of RelMetdataTest. Will work on null checker next... |
| @ThreadSafe | ||
| public class ReflectionToLambdaProvider implements LambdaProvider { | ||
|
|
||
| public static final ImmutableList<Source> DEFAULT_SOURCES = ImmutableList.<Source>builder() |
There was a problem hiding this comment.
If all handlers were refactored to only have 1 methods, couldn't we drive this off RelMetadataProvider, if a visiter pattern was exposed?
There was a problem hiding this comment.
This is effectively a reimagining of MetadataHandler as a functional interface with a more useful hierarchy/generic definition. Could we convert MetadataHandler to provide this functionality: yes, we probably could.
I haven't spent time trying to untangle the way we would get there. I would expect that it would take many releases to try minimize disruption while slowly converting all the interfaces/impls/etc.
There was a problem hiding this comment.
I'll add, in general the whole pattern of arbitrary classes carrying arbitrary methods that name match and arguments kind-of match seems like something that wouldn't translate well to a lambda pattern (which is why the concrete RelNode argument is part of the generic interface definition in this lambda pattern).
There was a problem hiding this comment.
The Lambda API is leaking the dispatch implementation into the API for binding in custom implementations. These APIs should be separate. It is actually fairly easy to adept this approach to use handlers and not be distributive to down stream projects.
There was a problem hiding this comment.
Can you expound on your comments. They are a bit brief.
Lambda API is leaking the dispatch implementation into the API for binding in custom implementations
Please be more specific about what exactly you think is leaking where. I think we may have different perspectives of what functionality is trying to be provided and how. The design here is that the declaration of a specific MetadataLambda implementation gives all the information about what RelNode types it applies to and what metadata it provides. I haven't yet posted the simple provider that simply returns hardcoded lambdas, but that was part of the goal for this design.
It is actually fairly easy to adept this approach to use handlers
I assume you mean adapt. I can see how one could make MetadataLambdas pretend to be handlers with a bunch of boilerplate but I don't see why that is a good idea. What exactly would this buy? Using janino on top of lambdas?
and not be distributive to down stream projects.
I assume you mean disruptive? I don't see how any of these changes are disruptive. This is just an alternative way to create RelMetadataQuery instances and specify metadata (via lambdas).
FYI, I will be updating this patch that to also include my vision for how to handle custom metadata types without any reflection and without excessive subclassing.
There was a problem hiding this comment.
If this is not the purposed end api, then we should not waste too much time discussing this implementation. But just as the API for metadata retrieval should not expose internals, the API for registering metadata implementations. This has not been the case for the previous api.
If your purposed API for this is to register every method the RelNode type instead of discovering them reflectively, then my questions is why do we need some convoluted framework for handling the dispatch instead of just harding coding in the dispatch. Their is pretty much a one to one mapping at that point of the amount of configuration needed compare to explicitly maintaining the implementation.
|
Just wondering, does it have something in common with https://github.com/forax/exotic#structuralcall---javadoc ? |
Similar, but not the same. Here we're using LambdaMetafactory and actually convert the function call to a generic version. Core of that work is here. |
|
I'm curious, what is the reason why this PR is blocked? |
I was waiting for CALCITE-4942 to be resolved since it could potentially substantially reduce the amount of boilerplate and reimplementation in this patch. Note that I did get [ProxyingMetadataHandlerProvider|https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/metadata/ProxyingMetadataHandlerProvider.java] which allows one to use metadata without runtime compilation. It isn't fast but at least it is functional in things like GraalVM. |
I thought there was not agreement on how to go forward with CALCITE-4942. From what I recall their was disagreement on wether to expose the lambda implementation classes or to use the ProxyingMetadataHandlerProvider to bridge interfaces between the metadata query and the handler generator. |
I posted a comment on the JIRA for CALCITE-4942 in late December but never saw a response (afaik). |
|
Abandoning this work as there was too much contention around it. |
