fix(isthmus): preserve nullability in VirtualTableScan Calcite roundtrip #684
Conversation
benbellick
left a comment
benbellick
left a comment
There was a problem hiding this comment.
a few small comments but mostly looks good!!
| // convert type first to guarantee we can handle the value. | ||
| final Type type = typeConverter.toSubstrait(literal.getType()); | ||
| final boolean n = type.nullable(); | ||
| return convert(literal, type.nullable()); |
There was a problem hiding this comment.
I'm now wondering if it's the case that type.nullable() will always be false. If nullability of literals is always lost in calcite values, won't this always just be the default?
There was a problem hiding this comment.
No, null literals have nullable types in Calcite, so it's not always non-nullable.
I have added the nullLiteralRoundTrip test to show this.
isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java
Outdated
Show resolved
Hide resolved
benbellick
left a comment
benbellick
left a comment
There was a problem hiding this comment.
I think this is great, thanks for your effort! I left a few very minor style comments but it otherwise looks good to me. The style comments are generally about just preferring the shorter ExpressionCreator API.
| final Type type = typeConverter.toSubstrait(literal.getType()); | ||
| final Type typeWithNullability = | ||
| nullable ? TypeCreator.asNullable(type) : TypeCreator.asNotNullable(type); | ||
| return ExpressionCreator.typedNull(typeWithNullability); |
There was a problem hiding this comment.
I think that as of #686, it is the case that typedNull will actually throw an exception if it is passed a non-null type. Just commenting so you are aware that if the TypeCreator.asNotNullable case is hit here, an exception will be thrown.
There was a problem hiding this comment.
Thanks for sharing, I gave it a bit more thinking and I think it's fine and the thrown error will be clear enough, no need to be defensive here nor adjust the error message (which is what you are suggesting, IIUC)
isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java
Outdated
Show resolved
Hide resolved
isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java
Outdated
Show resolved
Hide resolved
isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java
Outdated
Show resolved
Hide resolved
Uses schema field nullability when converting Calcite LogicalValues literals back to Substrait, instead of deriving nullability from the RexLiteral type. Calcite's LogicalValues may have literals with non-nullable types even when the schema field is nullable. Previously this caused a mismatch during roundtrip conversion, as VirtualTableScan requires row field types to exactly match schema field types. Fixes substrait-io#683
asolimando
force-pushed
the
fix-issue-683-nullability-roundtrip
branch
from
a43ed81 to
f22bbcd
Compare
Thanks Ben, I have addressed the style comments, I took the liberty to force push as the PR was already approved, to be able to resolve conflicts with latest |
2 participants
Uses schema field nullability when converting Calcite
LogicalValuesliterals back to Substrait, instead of deriving nullability from theRexLiteraltype.Calcite's
LogicalValuesmay have literals with non-nullable types even when the schema field is nullable (Calcite infers the tightest possible type for literals, by design).Previously this caused a mismatch during roundtrip conversion, as
VirtualTableScanrequires row field types to exactly match schema field types.Fixes #683