feat(isthmus): convert Calcite RelRoot to Substrait Plan.Root#370
Conversation
vbarua
changed the title
| public static Rel convert(RelRoot root, SimpleExtension.ExtensionCollection extensions) { | ||
| return convert(root.rel, extensions, FEATURES_DEFAULT); | ||
| public static Plan.Root convert(RelRoot relRoot, SimpleExtension.ExtensionCollection extensions) { | ||
| return convert(relRoot, extensions, FEATURES_DEFAULT); |
There was a problem hiding this comment.
This is a breaking API change.
A CalciteRelRoot corresponds to POJO Plan.Root, not a POJO Rel.
| assertEquals(expectedNames, substraitRelRoot.getNames()); | ||
|
|
||
| org.apache.calcite.rel.RelRoot calciteRelRoot2 = substraitToCalcite.convert(substraitRelRoot); | ||
| assertEquals(expectedNames, calciteRelRoot2.validatedRowType.getFieldNames()); |
There was a problem hiding this comment.
I was hoping to be able to use to the new Plan.Root based assertFullRoundTrip to test this, but it doesn't quite work: #371
| Rel pojo1 = SubstraitRelVisitor.convert(relRoot, EXTENSION_COLLECTION); | ||
| List<RelRoot> relRoots = s.sqlToRelNode(query, creates); | ||
| assertEquals(1, relRoots.size()); | ||
| RelRoot relRoot1 = relRoots.get(0); |
There was a problem hiding this comment.
In practice we only ever generate a single RelRoot. Capturing this here and removing the looping to simplify our test code.
vbarua
changed the title
|
This is the other direction of #339, which was implemented by @nielspardon I added a test for this in e230dcc which failed in CI (as expected). The failure was Effectively, the names were lost when going from Calcite to Substrait and back because we were converting the Calcite RelRoot to a Substrait Rel, which has nowhere to stash the names. Calcite then generates names based on the table definition, and as the colums are capitalized it uses the capitalized names. |
vbarua
force-pushed
the
vbarua/fix-name-roundtripping
branch
from
fe400a6 to
2479476
Compare
…isitor RelRoots must be converted to Plan.Roots in order to ensure that names are handled correctly. BREAKING CHANGE: converting a Calcite RelRoot no longer produces a Substrait Rel
vbarua
force-pushed
the
vbarua/fix-name-roundtripping
branch
from
2479476 to
0e7ad03
Compare
bvolpato
left a comment
bvolpato
left a comment
There was a problem hiding this comment.
This makes sense + looks good to me, thanks!
The fix for the breaking change is quite easy to figure out too
3 participants
In the SubstraitRelVisitor, RelRoots must be converted to Plan.Roots in order to
ensure that names are handled correctly.
BREAKING CHANGE: converting a Calcite RelRoot no longer produces a Substrait Rel