Skip to content
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

feat: support for simple extensions dependencies #265

Merged
merged 4 commits into from
Nov 22, 2023

Conversation

jvanstraten
Copy link
Contributor

No description provided.

@jvanstraten
Copy link
Contributor Author

This one has been on my mental TODO list for a while now: it's currently impossible for an extension to define functions that make use of types from another extension. Some problems with that:

  • The Substrait core extensions are split over a whole bunch of files because, well, it'd be an absolute mess otherwise, but consumers that define their own extension types currently can't follow the same paradigm for any function that uses any extension type.
  • Consumer X may want to make use of the types defined by consumer Y for extra compatibility. For example, Arrow is/will be defining extension types for all the Arrow types that don't exist in Substrait core. Execution engines besides Arrow/Acero may want to define functions for those types as well, if they use Arrow's type system internally.

AFAICT, the only missing link for supporting this as far as the specification is concerned is the lack of a way to define and refer to extension URI dependencies in YAML, so that's what this PR adds.

@jacques-n
Copy link
Contributor

Seems reasonable. Do you want to also write up some markdown for this?

@jvanstraten
Copy link
Contributor Author

Yep, good point.

@CLAassistant
Copy link

CLAassistant commented Oct 6, 2022

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.


```yaml
dependencies:
ext: /extension_types.yaml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a valid URI?

Copy link
Member

@EpsilonPrime EpsilonPrime Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a valid URI reference which we tend to accept in implementations. To make this a better example we could just prepend file:.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made that change.

EpsilonPrime
EpsilonPrime previously approved these changes Sep 28, 2023
@EpsilonPrime EpsilonPrime merged commit f0ecf54 into substrait-io:main Nov 22, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants