Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Match method to FuncDefArgType interface #53

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

anshuldata
Copy link
Contributor

@anshuldata anshuldata commented Sep 2, 2024

  • Added MatchWithNullability and MatchWithoutNullability methods on FuncDefArgType interface. This method returns true if argument "t" is type compatible with this type otherwise it returns false
  • These two APIs are necessary considering Match behavior will differ based on function's nullability handling (MIRROR, DECLARED_OUTPUT, DISCRETE).
  • Nullability "MIRROR" and "DECLARED_OUTPUT" requires MatchWithoutNullability
  • Nullability "DISCRETE" will require MatchWithNullability
  • Also updated go version to 1.22 to fix golint error with reflect API TypeFor
  • In follow up PR I will add Match method on FunctionVariant interface

Copy link

codecov bot commented Sep 2, 2024

Codecov Report

Attention: Patch coverage is 93.90244% with 5 lines in your changes missing coverage. Please review.

Project coverage is 55.79%. Comparing base (4265a21) to head (2240ec9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
types/parameterized_single_integer_param_type.go 78.57% 2 Missing and 1 partial ⚠️
types/parameterized_map_type.go 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #53      +/-   ##
==========================================
+ Coverage   54.93%   55.79%   +0.86%     
==========================================
  Files          35       35              
  Lines        6697     6778      +81     
==========================================
+ Hits         3679     3782     +103     
+ Misses       2829     2806      -23     
- Partials      189      190       +1     

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

types/parameterized_types.go Outdated Show resolved Hide resolved
types/parameterized_types.go Outdated Show resolved Hide resolved
types/types.go Outdated Show resolved Hide resolved
types/interval_compound_type.go Outdated Show resolved Hide resolved
types/interval_year_month_type.go Outdated Show resolved Hide resolved
types/types.go Outdated Show resolved Hide resolved
@zeroshade
Copy link
Contributor

Using reflect.TypeFor requires updating the module to go1.22 instead of go1.21, that's why the linter is failing most likely

@anshuldata anshuldata changed the title Add Match method to Type interface <DRAFT>Add Match method to Type interface Sep 10, 2024
@anshuldata
Copy link
Contributor Author

I will un-draft once #52 is merged and is rebased for this PR

@anshuldata anshuldata marked this pull request as draft September 10, 2024 11:41
@anshuldata anshuldata force-pushed the anshul/AddMatchAPI branch 2 times, most recently from 00fb6fd to ae11628 Compare September 12, 2024 14:37
…terface

* Also updated go version to 1.22 to fix golint error with reflect API TypeFor
@anshuldata anshuldata changed the title <DRAFT>Add Match method to Type interface Add Match method to FuncDefArgType interface Sep 12, 2024
@anshuldata anshuldata marked this pull request as ready for review September 12, 2024 18:24
Copy link
Contributor

@jacques-n jacques-n left a comment

Choose a reason for hiding this comment

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

Feels like it is pretty clean and simple now and all outstanding requested changes have been dealt with.

The biggest controversial thing is upgrading to go 1.22. Looked at only 1.22 and 1.23 are now supported by community so that seems fine.

@jacques-n jacques-n merged commit c48fb53 into substrait-io:main Sep 13, 2024
8 checks passed
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.

3 participants