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: add derivation expression evaluator #63

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

tokoko
Copy link
Contributor

@tokoko tokoko commented Oct 5, 2024

Adds support for the evaluation of extension function's return type expressions.
It's meant to be used by producers to correctly infer return types.
It uses antlr grammar (temporarily) copy-pasted from substrait-java w/o changes.

Copy link

github-actions bot commented Oct 5, 2024

ACTION NEEDED

Substrait follows the Conventional Commits
specification
for
release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

@tokoko
Copy link
Contributor Author

tokoko commented Oct 5, 2024

Evaluation is not exhaustive yet, I've implemented the types necessary for functions in ibis-substrait and subframe.

Some open questions:

  • The term "derivation expression" was borrowed from proto message type describing the concept although the proto itself isn't being used. I'm open to renaming it if there are better alternatives...
  • I added antlr4-python3-runtime as required dependency. Should I make it optional?

@tokoko
Copy link
Contributor Author

tokoko commented Oct 5, 2024

@jacques-n FYI

@gforsyth
Copy link
Member

gforsyth commented Oct 5, 2024

I'll try to take a detailed look at this next week (or maybe tomorrow?), but some initial thoughts:

I think leaving the antlr runtime as a required dependency is fine. It's very light-weight, and the likelihood of a given plan not having any scalar functions seems low.

And we should mark the antlr-generated files with linguist-generated

@tokoko
Copy link
Contributor Author

tokoko commented Oct 6, 2024

It's very light-weight, and the likelihood of a given plan not having any scalar functions seems low.

true, but note that this is likely only necessary for producers as output types of ScalarFunction calls are already in the substrait plan, therefore no derivation is needed for a consumer. So we would be adding unnecessary dependency to anyone that's just consuming.

And we should mark the antlr-generated files with linguist-generated

checked and src/substrait/gen/** (parent directory) is already marked as such

@gforsyth
Copy link
Member

gforsyth commented Oct 7, 2024

true, but note that this is likely only necessary for producers as output types of ScalarFunction calls are already in the substrait plan, therefore no derivation is needed for a consumer. So we would be adding unnecessary dependency to anyone that's just consuming.

Ahh, right. OK, well, I'm good with making it an optional dependency.

@tokoko
Copy link
Contributor Author

tokoko commented Oct 9, 2024

I'm thinking of calling the extra extensions as the forthcoming ExtensionRegistry (next PR) will almost always be how this functionality is accessed by the users. Plus it will also require other dependencies (mainly pyyaml), simpler to put them both in a single extra.

grammar SubstraitType;

//
fragment A : [aA];
Copy link
Member

Choose a reason for hiding this comment

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

When we move this into core we should use:

options {
    caseInsensitive = true;
}

so we can eliminate all these fragments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can open an issue in substrait-java for this..

@tokoko
Copy link
Contributor Author

tokoko commented Oct 20, 2024

@gforsyth just a reminder

Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

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

Thanks for the ping @tokoko and sorry for the delay in getting to this.

@gforsyth gforsyth enabled auto-merge (squash) October 28, 2024 21:17
@gforsyth gforsyth merged commit b78a614 into substrait-io:main Oct 28, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants