feat!: require compound functions names in extension references #537
Conversation
|
ACTION NEEDED Substrait follows the Conventional Commits The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
|
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). |
vbarua
changed the title
vbarua
force-pushed
the
vbarua/require-types-in-function-names
branch
from
f7ef283
to
2595292
Compare
Co-authored-by: Weston Pace <[email protected]>
Co-authored-by: Weston Pace <[email protected]>
|
I'm good with this change |
There was a problem hiding this comment.
This is "technically" a breaking change. Although it's not actually a proto change so I doubt it will affect too many people. However, it would be good to label it as a breaking change. Can we do that real quick? Then I approve and we can merge.
@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.
|
@westonpace |
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. |
|
Thanks for writing up this change! |
#537 clarified that compound extension signatures are now required. I noticed that the table specifying how to type those signatures didn't include booleans -- I _think_ it's probably `bool` but either way it should be included. Also in this PR, a quick indentation fix so that the `Note:` admonitions render correctly (there needs to be blank line and then an indented block)
BREAKING CHANGE: plans referencing functions using simple
names (e.g. not vs not:bool) will no longer be valid.