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 proto <-> pojo custom type conversion #144

Merged
merged 3 commits into from
May 10, 2023

Conversation

vbarua
Copy link
Member

@vbarua vbarua commented Apr 25, 2023

Adds machinery for reading type extensions and handling their conversion in POJOs to proto form and back.

All changes were motivated by the example in TestTypeRoundtrip, which:

  • Reads user defined types from an extension
  • Reads function using those types from the extension
  • Builds a POJO using those types and functions.
  • Converts the POJO -> Proto -> POJO and checks that the two POJOs are the same.

The majority of changes stem come from:

  • Adding the new io.substrait.type.Type.UserDefined to the type visitors.
  • Adding code to resolve and lookup types within a plan context.
  • Updating static usages of TypeProtoConverter to instance specific usages to perform said resolution and lookup. TypeProtoConverter can no longer be static as resolution and lookup are plan specific.

There a number of classes w/ TODOs to apply renames and/or moves. I held off on applying them in this PR to keep the diff smaller and also check w/ the community to see if they were reasonable before investing time in them.

@vbarua vbarua force-pushed the vbarua/type-extension-handling branch from dfc456a to 91996dc Compare April 25, 2023 22:44
@vbarua vbarua force-pushed the vbarua/type-extension-handling branch 2 times, most recently from 7de31c6 to f70a761 Compare April 25, 2023 23:52
@vbarua vbarua marked this pull request as ready for review April 26, 2023 16:38
@vbarua
Copy link
Member Author

vbarua commented Apr 26, 2023

CI is failing TestTypeParser tests, however when I run ./gradlew clean test I don't see any failures.
Thoughts?

> Task :core:test

TestTypeParser > parameterizedSimple() FAILED
    java.lang.NullPointerException at TestTypeParser.java:76

TestTypeParser > derivationSimple() FAILED
    java.lang.NullPointerException at TestTypeParser.java:76

TestTypeParser > compound() FAILED
    java.lang.NullPointerException at TestTypeParser.java:95

TestTypeParser > parameterizedCompound() FAILED
    java.lang.NullPointerException at TestTypeParser.java:95

TestTypeParser > basic() FAILED
    java.lang.NullPointerException at TestTypeParser.java:76

TestTypeParser > derivationParameterized() FAILED
    java.lang.NullPointerException at TestTypeParser.java:115

TestTypeParser > derivationCompound() FAILED
    java.lang.NullPointerException at TestTypeParser.java:95

TestTypeParser > parameterizedParameterized() FAILED
    java.lang.NullPointerException at TestTypeParser.java:115

@JsonDeserialize(as = ImmutableSimpleExtension.FunctionSignatures.class)
@JsonSerialize(as = ImmutableSimpleExtension.FunctionSignatures.class)
@Value.Immutable
public abstract static class FunctionSignatures {
// TODO: Rename to ExtensionSignatures ???
Copy link
Member Author

Choose a reason for hiding this comment

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

I want to move away from FunctionSignature as this now has types in it as well, but I'm not sure if ExtensionSignature is a good name either. I'm open to suggestions if anyone has a better alternative.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds reasonable

@vbarua vbarua force-pushed the vbarua/type-extension-handling branch from 35ffc02 to b6ecf31 Compare April 27, 2023 15:27
@vbarua
Copy link
Member Author

vbarua commented Apr 27, 2023

Running tests using ./gradlew clean test --no-build-cache let me identify the CI issue. Fixed in b6ecf31

@JsonDeserialize(as = ImmutableSimpleExtension.FunctionSignatures.class)
@JsonSerialize(as = ImmutableSimpleExtension.FunctionSignatures.class)
@Value.Immutable
public abstract static class FunctionSignatures {
// TODO: Rename to ExtensionSignatures ???
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds reasonable

@jacques-n jacques-n merged commit 9a12e60 into substrait-io:main May 10, 2023
ajegou pushed a commit to ajegou/substrait-java that referenced this pull request Mar 29, 2024
* feat: support proto <-> pojo custom type conversion

* test: fix NPE when using Type.NULLABLE/REQUIRED in test

---------

Co-authored-by: Jacques Nadeau <jacques@apache.org>
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