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

feat: support user-defined types in YAML extensions #60

Merged
merged 7 commits into from
Oct 30, 2024

Conversation

kadinrabo
Copy link
Contributor

Support parsing UserDefinedType (denoted by u!) in extension yaml

@kadinrabo kadinrabo marked this pull request as ready for review October 10, 2024 20:27
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.53%. Comparing base (002720e) to head (dd2fc7d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #60      +/-   ##
==========================================
- Coverage   55.32%   54.53%   -0.80%     
==========================================
  Files          35       35              
  Lines        6924     8006    +1082     
==========================================
+ Hits         3831     4366     +535     
- Misses       2896     3441     +545     
- Partials      197      199       +2     

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

Copy link
Member

@vbarua vbarua left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, but I'm not super familiar with the Go side of things.

cc: @zeroshade, @jacques-n: should be an easy review.

extensions/simple_extension_test.go Show resolved Hide resolved
types/parser/type_parser.go Show resolved Hide resolved
@jacques-n
Copy link
Contributor

I believe @anshuldata is or will be working on a patch to replace the hand parsing with antlr grammar. That should address several gaps in the current function parsing. I'd be inclined to wait until that is merged before trying to solve this gap (as some of it may come for cheap/free).

@vbarua
Copy link
Member

vbarua commented Oct 11, 2024

@jacques-n do you think that will be available next week? We have some stuff internally that's blocked on us being able to handle user-defined types. Aside from potentially redundant work on our end, would there be any harm in updating the hand parser.

@zeroshade
Copy link
Contributor

@vbarua It's unlikely that something as large as replacing with an antlr grammar will be merged next week as I'm sure that would need significant review and testing. My personal opinion would be that it makes sense to update this if you need it. I doubt there's any harm besides the redundant work.

@jacques-n thoughts?

@jacques-n
Copy link
Contributor

I think this patch is building debt on debt.

In general, I find it to be a bad habit to accept something we know we're going to delete in a couple of weeks (unless we're fixing a critical regression). It sets a bad precedent for new contributors on how we work. If we know we're going in a certain direction and one contributor wants to go there faster than another, let's just have them take us to the target outcome.

I'm -0 on this patch. I'm not going to block.

@kadinrabo
Copy link
Contributor Author

I honestly agree this PR doesn't really solve much with antlr on the way and isn't a good precedent. Thankfully this is small. Although, I want to keep contributing positively so I'll add this to my personal "developing substrait" notes. At this point I guess I'm just asking for a ✅ out of kindness for a new contributor :) as this would significantly unblock me internally.
cc @zeroshade @jacques-n

Copy link
Contributor

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

LGTM assuming that @jacques-n has no objections

@vbarua
Copy link
Member

vbarua commented Oct 30, 2024

Based on

I'm -0 on this patch. I'm not going to block.

I'm going to merge this in.

@vbarua vbarua changed the title feat: support custom types in extension yamls feat: support user-defined types in YAML extensions Oct 30, 2024
@vbarua vbarua merged commit 9cb5896 into substrait-io:main Oct 30, 2024
7 of 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.

4 participants