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: opening ProtoRelConverter for extension #285

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

bvolpato
Copy link
Member

@bvolpato bvolpato commented Aug 9, 2024

I have a need to override some of the behavior in ProtoRelConverter.java's newExtensionTable, but today I can't because the class was designed to be all private.

I'd like to make that open if possible, to allow easily extending it instead of having to duplicate any code. I'm aware that there are some extension points / customization entries such as detailFromExtensionTable, but that signature isn't enough to have roundtrip-able leafs, because I can only derive the schema from the Rel itself, not from the data packed in the Any instance.

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.

This looks reasonable to me. It should allow consumers to extend the ProtoRelConverter behaviour more easily.

@vbarua vbarua merged commit 3864710 into substrait-io:main Aug 9, 2024
12 checks passed
@bvolpato bvolpato deleted the open-proto-rel-converter branch August 9, 2024 19:58
@jacques-n
Copy link
Collaborator

I generally suggest using composition instead of subclassing. Exposing these methods as protected creates some weird testing patterns as things progress. I'd be much more inclined to say let's convert to interface and final this class.

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