Skip to content

Add lambda substrait support#21193

Open
gstvg wants to merge 22 commits into
apache:mainfrom
gstvg:lambda_substrait
Open

Add lambda substrait support#21193
gstvg wants to merge 22 commits into
apache:mainfrom
gstvg:lambda_substrait

Conversation

@gstvg
Copy link
Copy Markdown
Contributor

@gstvg gstvg commented Mar 27, 2026 •
edited
Loading

Which issue does this PR close?

Part of #21172

Rationale for this change

Substrait support wasn't implemented in the core lambda support to reduce PR size

What changes are included in this PR?

Substrait consuming and producing of higher-order functions, lambdas and lambda variables

Are these changes tested?

Unit tests added to datafusion/substrait/tests/cases/roundtrip_logical_plan.rs

Are there any user-facing changes?

None

@github-actions github-actions Bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) substrait Changes to the substrait crate catalog Related to the catalog crate common Related to common crate execution Related to the execution crate proto Related to proto crate functions Changes to functions implementation datasource Changes to the datasource crate ffi Changes to the ffi crate labels Mar 27, 2026
@gstvg gstvg mentioned this pull request Mar 27, 2026
Open
27 tasks
@benbellick
Copy link
Copy Markdown
Contributor

benbellick commented Apr 28, 2026

👋 Hello from @substrait-io. Great to see the core lambda PR has gotten through! Once this PR is in a ready to review state and is rebased off of main, I will be more than happy to help review it 🙂

@gstvg
Copy link
Copy Markdown
Contributor Author

gstvg commented Apr 29, 2026

Thanks @benbellick, I will open this tonight. Besides rebasing, I believe it misses some tests (I tested with sqllogictests only)

@github-actions github-actions Bot added documentation Improvements or additions to documentation spark and removed documentation Improvements or additions to documentation sql SQL Planner logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) catalog Related to the catalog crate common Related to common crate execution Related to the execution crate proto Related to proto crate functions Changes to functions implementation labels Apr 29, 2026
benbellick
benbellick reviewed May 11, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@benbellick benbellick left a comment

Choose a reason for hiding this comment

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

Few more comments but on the whole this looks good to me! Thanks

Comment thread datafusion/substrait/tests/testdata/test_plans/higher_order_function.json Outdated
Comment thread datafusion/substrait/tests/cases/serialize.rs Outdated
Comment thread datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs
Comment thread datafusion/substrait/src/logical_plan/consumer/expr/lambda.rs
benbellick
benbellick approved these changes May 13, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@benbellick benbellick left a comment

Choose a reason for hiding this comment

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

LGTM, great work! Excited to get this in 🚀

Feel free to summon the maintainers now.

Comment thread datafusion/substrait/src/logical_plan/consumer/substrait_consumer.rs
Comment thread datafusion/substrait/tests/cases/serialize.rs
@gstvg
Copy link
Copy Markdown
Contributor Author

gstvg commented May 14, 2026

LGTM, great work! Excited to get this in 🚀

Thanks for the review @benbellick
cc @rluvaton @comphead this is ready for the final review

@rluvaton
Copy link
Copy Markdown
Member

rluvaton commented May 14, 2026

I'm unfamiliar with substrait except for the idea, so I don't know if I can have meaningful review

@rluvaton
Copy link
Copy Markdown
Member

rluvaton commented May 14, 2026

@gstvg according to the breaking change detector there are no breaking change api wise in this pr, is your commemt still correct?

Are there any user-facing changes?

Yes, there are breaking changes, new methods without default implementation have been added to SubstraitConsumer and SubstraitProducer

@gstvg
Copy link
Copy Markdown
Contributor Author

gstvg commented May 15, 2026

@rluvaton, indeed, after applying @benbellick suggestions there's no breaking changes anymore, description updated, thanks

@gstvg
Copy link
Copy Markdown
Contributor Author

gstvg commented May 18, 2026

Hey @gabotechs, I see you worked on some substrait PRs, bringing you here in case you have interest in this. thanks

gabotechs
gabotechs approved these changes May 19, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@gabotechs gabotechs left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good! gave a shallow pass and just made one suggestion, but I trust @benbellick in his review (he's a work colleague). Thanks @gstvg for your work with lambdas 🙏

Comment thread datafusion/substrait/src/extensions.rs Outdated
@gstvg
Copy link
Copy Markdown
Contributor Author

gstvg commented May 19, 2026 •
edited
Loading

Thanks @gabotechs. The derive(Clone) is removed. There's only two comments from @benbellick that were outside substrait translation and weren't fully addressed, #21193 (review) and #21193 (comment)

@gabotechs
Copy link
Copy Markdown
Contributor

gabotechs commented May 19, 2026

There's only two comments from @benbellick that were outside substrait translation and weren't fully addressed, #21193 (review) and #21193 (comment)

Answered there

gstvg
gstvg commented May 19, 2026
View reviewed changes
Comment thread datafusion/substrait/tests/cases/logical_plans.rs
@gstvg
Copy link
Copy Markdown
Contributor Author

gstvg commented May 20, 2026

Answered there

@gabotechs the one that required a change should be resolved now

@gstvg gstvg mentioned this pull request May 20, 2026
Open
47 tasks
@gabotechs
Copy link
Copy Markdown
Contributor

gabotechs commented May 26, 2026

This one is ready to go, thanks @gstvg! and also thanks @benbellick for the review.

@gabotechs gabotechs added this pull request to the merge queue May 26, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 26, 2026
@gabotechs
Copy link
Copy Markdown
Contributor

gabotechs commented May 26, 2026

@gstvg it seems like this branch needs to be updated with main, as there seems to be a compilation error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gabotechs gabotechs gabotechs approved these changes

+1 more reviewer

@benbellick benbellick benbellick approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api change Changes the API exposed to users of the crate substrait Changes to the substrait crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gstvg @benbellick @rluvaton @gabotechs