feat: support UDT literals using bytestring/binary representation in calcite #232
Conversation
|
|
cheikhachraf
changed the title
|
Thanks for working on this! One thing I think would be helpful would be to add a full roundtrip test to verify that Substrait inputs with user-defined type literals can go from There's an example for testing user-defined types in CustomFunctionTest, it could make sense to add a test to this class (maybe with a rename) or make a new class. There is code for something like this in assertFullRoundTrip(Rel pojo1), however that assumes that only standard extensions and functions are loaded (for now). |
vbarua
changed the title
core/src/main/java/io/substrait/expression/proto/ExpressionProtoConverter.java
Outdated
Show resolved
Hide resolved
cheikhachraf
force-pushed
the
achraf.cheikhmohamed/add-support-for-udt=literals
branch
from
5b8f857 to
9329550
Compare
There was a problem hiding this comment.
Nice work finding the REINTERPRET code in Calcite, that works really well for this ✨
Overall the changes look good. I did spruce things up a bit by:
- Updating the test to include a check going from POJO -> PROTO -> POJO
- Turned your comment into a docstring on the REINTERPRET converter to be able to link to the code in question.
| var binaryLiteral = rexBuilder.makeBinaryLiteral(new ByteString(expr.value().toByteArray())); | ||
| return rexBuilder.makeReinterpretCast( | ||
| typeConverter.toCalcite(typeFactory, expr.getType()), binaryLiteral, null); | ||
| } |
There was a problem hiding this comment.
This works really well! We can store the binary data blob AND wrap it with the correct type ✨
This commit re-implements the UDT literal conversion to calcite using REINTERPRET, a calcite sql special operator, wrapped around the binary literal. This is needed to pass Calcite type checking.
vbarua
force-pushed
the
achraf.cheikhmohamed/add-support-for-udt=literals
branch
from
786a010 to
fed5a7c
Compare
|
We've had enough activity this week for there to have been merge conflicts 😅 |
substrait-java AND isthmus can now handle user-defined type literals
calcite does not accept RelDatatypes with sqltypename.OTHER and has no way of representing it as of now.
It will reject it at different steps such as RexLiteral Precondition type check or when building using the RexBuilder.
There's a ticket for this on Calcite's board but nothing as of of now.
one hacky solution would be: