search

substrait-io / substrait-java

Apache License 2.0
75 stars 72 forks source link

feat!: handle user-defined types in Isthmus #149

Closed vbarua closed 1 year ago

vbarua commented 1 year ago

BREAKING CHANGE: TypeConverter no longer uses static methods BREAKING CHANGE: SimpleExtension.MAPPER has been replaced with SimpleExtension.objectMapper(String namespace)

vbarua commented 1 year ago

Change Summary

Updated the TypeConverter to accept a UserTypeMapper, an interface which implements type conversions between Substrait and Calcite. The existing code made heavy use of the static TypeConverter.convert method, which didn't work well for allowing users to include their own UserTypeMapper. I've updated all call sites to use an instance of TypeConverter, and most of the this code in this PR is for propagating the TypeConverter through the system.

As part of this change, I renamed convert to toSubstrait and toCalcite for improved clarity (the original method had an overload for each side of the conversion).

Additionally, I've simplified the resolution of URIs for types by injecting the URI directly into the ObjectMapper. This came up because types in FuncArgs were being created without URIs, and propagating the resolution to them was harder than just getting rid of it entirely.

See CustomFunctionTest.java for an example of how all the parts fits together.

vbarua commented 1 year ago

Feels like it is better to get this in and iterate if we figure out it needs improvements later.

On my end, I'm more than happy to make any fixes and improvements as the need arises 🙇