Skip to content

fix(types): preserve nested nullability in any binding - #244

Draft
benbellick wants to merge 6 commits into
mainfrom
fix/any-nullability-binding
Draft

fix(types): preserve nested nullability in any binding#244
benbellick wants to merge 6 commits into
mainfrom
fix/any-nullability-binding

Conversation

@benbellick

Copy link
Copy Markdown
Member

Function matching for constrained any parameters was ignoring nullability too broadly. Under the clarified Substrait semantics, MIRROR and DECLARED_OUTPUT strip only the outermost argument nullability before binding; nested nullability inside compound types still participates in structural matching.

This keeps cases like list<i32> and list<i32?> from binding to the same list<any1> parameter while still allowing outer nullable wrappers to match as intended. The added tests mirror the examples from substrait-io/substrait#943.


Note: This PR was developed with AI assistance. All changes have been reviewed, and I take full responsibility for this contribution.

@codecov

codecov Bot commented May 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.65079% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.08%. Comparing base (5073366) to head (ad26157).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
extensions/variants.go 95.12% 1 Missing and 1 partial ⚠️
types/parameterized_func_type.go 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #244      +/-   ##
==========================================
+ Coverage   68.84%   69.08%   +0.23%     
==========================================
  Files          47       47              
  Lines       10862    10896      +34     
==========================================
+ Hits         7478     7527      +49     
+ Misses       3030     3015      -15     
  Partials      354      354              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dd-david-levin

Copy link
Copy Markdown

I opened #291 to cover a complementary return-substitution gap.

#244 correctly preserves/enforces nested nullability during matching, but unwrapAnyTypeWithName still returns the nullable concrete occurrence directly. As a result, map(list<any1?>, list<any2?>) -> map<any1, any2?> resolves as map<string?, i64?> rather than map<string, i64?>.

#291 normalizes a nullable anyN? source occurrence to the required base binding and lets the return occurrence reapply its own nullability. I also verified the #244 merge ref plus #291 with go test ./...; they pass together.

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.

2 participants