fix(core): correct type short names for binary and fixedbinary#826
Merged
fix(core): correct type short names for binary and fixedbinary#826
Conversation
…ypeString Type.Binary was emitting "binary" instead of the spec-mandated short name "vbin", and Type.FixedBinary / ParameterizedType.FixedBinary were emitting "fbinary" instead of "fbin". These mismatches caused function signature key lookup failures for any function with binary or fixedbinary arguments. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a parameterized test suite that explicitly asserts each type maps to its canonical short name from https://substrait.io/extensions/#type-short-names, covering both required and nullable variants plus UserDefined and any/lossless edge cases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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. |
vbarua
changed the title
vbarua
commented
Apr 30, 2026
| Arguments.of(c.struct(c.I32), "struct"), | ||
| Arguments.of(c.list(c.I32), "list"), | ||
| Arguments.of(c.map(c.I32, c.I64), "map"))); | ||
| } |
Member
Author
There was a problem hiding this comment.
Not the most interesting test in world, but wanted to have something in the harness that would catch changes to these mappings.
vbarua
force-pushed
the
vbarua/short-type-name-fixes
branch
from
4b99558 to
b63e4a3
Compare
bvolpato
approved these changes
May 1, 2026
Member
bvolpato
left a comment
bvolpato
left a comment
There was a problem hiding this comment.
LGTM, thanks Victor!
nit: adding an integration test to the function-signature key generation path itself could be a good follow-up, instead of just focusing on the unit here
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type.Binary was emitting "binary" instead of the spec-mandated short name
"vbin", and Type.FixedBinary / ParameterizedType.FixedBinary were emitting
"fbinary" instead of "fbin". These mismatches caused function signature key
lookup failures for any function with binary or fixedbinary arguments.