feat(isthmus): support converting Substrait Plan.Root to Calcite RelRoot#339
Conversation
nielspardon
force-pushed
the
par-convert-root
branch
from
4f305d2 to
17f131b
Compare
nielspardon
force-pushed
the
par-convert-root
branch
from
17f131b to
289fa5a
Compare
nielspardon
force-pushed
the
par-convert-root
branch
2 times, most recently
from
6524fc8 to
1a2c794
Compare
|
|
||
| public class SubstraitToCalciteTest extends PlanTestBase { | ||
| @Test | ||
| void testConvertRoot() throws Exception { |
There was a problem hiding this comment.
might be nice to split this into separate test functions 😄
There was a problem hiding this comment.
sure. I can split it up
nielspardon
force-pushed
the
par-convert-root
branch
from
1a2c794 to
41d35d5
Compare
Blizzara
left a comment
Blizzara
left a comment
There was a problem hiding this comment.
Thanks - this makes sense to me! However I'm not using Calcite myself, @vbarua I think you might, do you wanna check this through as well?
Just to state the status if I understand it correctly: this correctly reads the proper DFS names lists, but only renames the top-level fields in Calcite. I think that's still an improvement, so fine to merge as it is and improve on it, but let me know if you think otherwise.
Correct, it stores all the field names from the Substrait Plan.Root in the Calcite RelRoot. The only caveat is that Calcite currently only applies the names from the top-level. With a Calcite fix it could use the information provided in the provided row type to also apply the aliases for nested fields. And yes, it is still an improvement since currently substrait-java didn't even provide the top-level names to Calcite. |
There was a problem hiding this comment.
Overall this looks reasonable to me. Left a number of minor comments. Feel free to address any that make sense to you @nielspardon. I can take another look tomorrow and merge this in.
| ImmutableRoot.builder() | ||
| .input( | ||
| substraitBuilder.namedScan( | ||
| Collections.singleton("stores"), |
There was a problem hiding this comment.
minor/opinionated: I would use List.of("stores") here. For table names, order matters, think tpcds.store vs store.tpcds. Collections.singleton returns a Set.
Now, none of this actually matters if there's only a single name, like in this case, but we use List.of(<single_name>) in a bunch of other places and it would be nice to be consistent.
This applies to you other usages of Collections.singleton as well.
There was a problem hiding this comment.
Makes sense to me. Do you think the use of the Iterable type in SubstraitBuilder should be reconsidered to reflect that the order matters for some of these types?
There was a problem hiding this comment.
I changed the Collections.singleton() to either List.of() or Set.of().
There was a problem hiding this comment.
Do you think the use of the Iterable type in SubstraitBuilder should be reconsidered to reflect that the order matters for some of these types?
That's actually a really good idea. Very much beyond the scope of this PR.
nielspardon
force-pushed
the
par-convert-root
branch
2 times, most recently
from
b55efba to
67b5f9e
Compare
Signed-off-by: Niels Pardon <par@zurich.ibm.com>
nielspardon
force-pushed
the
par-convert-root
branch
from
6db0add to
3be3b9a
Compare
|
Thanks for working on this @nielspardon! |
3 participants
Isthmus currently only supports converting a Substrait
Relinto a CalciteRelNodelosing the final column names which are encoded in the SubstraitPlan.Rootand which should be converted into a CalciteRelRoot.This PR adds a
convert()method for converting a SubstraitPlan.Rootinto a CalciteRelRootpreserving the final column names.Currently, substrait-java does not support DML/DDL relations like
WriteRelso the conversion may have to be revisited once support for DML/DDL relations is being added.