-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DO NOT MERGE] Draft of PartiQL SPI factoring for functions #1332
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great step in the right direction! It's certainly what we want.
Somewhat related to this PR due to the refactoring of SPI APIs, I've been recently thinking about how we can maintain the separation of concerns for each stage in the pipeline. It seems like PartiQLPlanner
(library) is the only thing that should use ConnectorMetadata
. PartiQLEval
(library) will only deal with loading in data/functions (aka ConnectorBindings
and ConnectorFunctions
). The PartiQL CLI
(application) is the only thing that cares about the Plugin
. It might be a good idea to extract those APIs, place them where they are needed (to maintain the separation of concerns), and flip the dependency tree so that SPI is a direct dependency of the CLI and the CLI only. Note: this isn't a comment about your PR as your PR is doing something different, but your work made me think of it.
Anyways, great work! I'm in line with the changes that you've made.
partiql-spi/src/main/kotlin/org/partiql/spi/connector/ConnectorBindings.kt
Outdated
Show resolved
Hide resolved
partiql-spi/src/main/kotlin/org/partiql/spi/connector/ConnectorMetadata.kt
Outdated
Show resolved
Hide resolved
partiql-spi/src/main/kotlin/org/partiql/spi/connector/ConnectorMetadata.kt
Outdated
Show resolved
Hide resolved
partiql-spi/src/main/kotlin/org/partiql/spi/connector/ConnectorMetadata.kt
Outdated
Show resolved
Hide resolved
…rMetadata.kt Co-authored-by: John Ed Quinn <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mental model says there are two entity modeled, functions and values.
For functions, we use the ConnectorFunction
interface, which holds the function signatures and concrete implementation.
For values, we use ConnectorObject
, which is an empty interface for metadata. and ConnectorBinding
to hold the concrete value.
I found this difference in modeling a little inconsistent. Could you please elaborate on the decision here?
public val absolutePath: ConnectorObjectPath, | ||
public val value: ConnectorObject | ||
public val path: ConnectorPath, | ||
public val obj: ConnectorObject, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the need of a separate ConnectorObject
interface, if this is just going to be holding metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConnectorObject is for arbitrary metadata about a catalog's values. ConnectorFunctionHandle is only for function metadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your question is actually slightly different.
ConnectorObject is not analogous to ConnectorFunction. ConnectorObject would be better named as ConnectorObjectMetadata. It is most similar to FunctionSignature, but can be anything.
This PR is a draft of the interfaces for introducing user-defined functions to a catalog via a Connector. ConnectorMetadata can be queried (analogous to objects) for function types (function signatures). The connector also provides ConnectorFunctions which is responsible for getting function implementations from a handle.
and Code Style Guidelines? [YES/NO]
YES
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.