substrait-io / substrait

A cross platform way to express data transformation, relational algebra, standardized record expression and plans.
https://substrait.io
Apache License 2.0
1.2k stars 156 forks source link

feat!: require compound functions names in extension references #537

Closed vbarua closed 1 year ago

vbarua commented 1 year ago

BREAKING CHANGE: plans referencing functions using simple names (e.g. not vs not:bool) will no longer be valid.

github-actions[bot] commented 1 year ago

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.

vbarua commented 1 year ago

As discussed in Slack, this PR removes the ability to use simple names in function references. This change makes function lookup easier from an implementor perspective.

It also helps avoid a potential pitfall where, for a function with only a single implementation, adding an implementation becomes a breaking change as an plan that referenced that function using its simple name is no longer valid (as the function reference is now ambiguous).

jacques-n commented 1 year ago

I'm good with this change

vbarua commented 1 year ago

@westonpace Added a BREAKING CHANGE to the commit message. The feat! should also mark this as a breaking change. Is that what you had in mind, or is there something else I need to do?

mbrobbel commented 1 year ago

@mbrobbel we might need to update the validator to be more strict now (and can probably simplify some things in the process). Let me know if I can help.

Yes, this being more strict simplifies the validation logic. I haven't had much time lately to make progress on the parser, but I do plan on continuing the effort. I already have a TODO for this, because I didn't implement compound name parsing yet.

westonpace commented 1 year ago

Thanks for writing up this change!