feat: support output order remapping #132
Conversation
vbarua
commented
Mar 6, 2023
•
vbarua
commented
- Makes fields in SubstraitRelNodeConverter protected to allow for re-use when extending it with custom behaviours.
- Adds deriveRecordType to Cross to make it consistent with other Rels.
- Adds SubstraitBuilder, which encodes a DSL to make it easier to build plans programmatically.
- Adds SubstraitToCalcite, to convert directly from Substrait to Calcite. Includes functionality to extract a schema from the Substrait Rel directly.
- Bumps the junit-jupiter version for access to the assertInstanceOf method.
- Adds tests to verify the layout of Rels when converted w/ both Direct and Remap output ordering.
- Adds support for the Remap output type to SubstraitRelNodeConverter.
Access to the various converters and builders is useful for users extending SubstraitRelNodeConverter with custom behaviour
This makes Cross consistent w/ other Rels
| import java.util.stream.Collectors; | ||
| import java.util.stream.Stream; | ||
|
|
||
| public class SubstraitBuilder { |
There was a problem hiding this comment.
See SubstraitRelNodeConverter for an example of how the methods in this class are used.
Could make this internal only as well to limit committing to an external API for this.
There was a problem hiding this comment.
I like the idea of moving towards an easier to write wrapper for generating plans, but any "usability" API would almost certainly need to come with proper docs, ideally containing short usage examples. An undocumented usability API, or one only documented by having to read the source code of other code, is going to be of limited use.
There was a problem hiding this comment.
This got merged before I circled back to hiding this.
Looking at https://substrait.io/spec/versioning/, because we're pre-1.0 I'm more comfortable with having it in. It is super useful for writing tests, both in substream-java and downstream.
but any "usability" API would almost certainly need to come with proper docs, ideally containing short usage examples.
100% something to have in place before 1.0.
isthmus/src/test/java/io/substrait/isthmus/SubstraitRelNodeConverterTest.java
Show resolved
Hide resolved
| import org.immutables.value.Value; | ||
|
|
||
| @Value.Immutable | ||
| public abstract class Cross extends BiRel { | ||
|
|
||
| @Override | ||
| protected Type.Struct deriveRecordType() { |
There was a problem hiding this comment.
I've realised that adding this removes the deriveRecordType method from the ImmutableCross.Builder, meaning this is technically a breaking change.
| * <p>Can be extended to customize the {@link RelBuilder} and {@link SubstraitRelNodeConverter} used | ||
| * in the conversion. | ||
| */ | ||
| public class SubstraitToCalcite { |
There was a problem hiding this comment.
Thinking about this some more, though I'm pretty happy with this I don't know if I'm "happy as public API forever" kind of happy. Could make this protected and keep it internal only for now.
There was a problem hiding this comment.
I think if you're uncertain, it might be best to be conservative and leave it protected until someone needs it.
There was a problem hiding this comment.
This got merged before I circled back to hiding this.
Looking at https://substrait.io/spec/versioning/, because we're pre-1.0 I'm more comfortable with having it in. It is actually helpful to have as downstream user and it does make it easier for folks experiment with converting substrait plans directly to Calcite.
* refactor: expand access to SubstraitRelNodeConverter fields * feat: add deriveRecordType to Cross * feat: substrait builder dsl * feat: self-contained Substrait to Calcite converter * chore: bump junit-jupiter * test: check for application of remappings * feat: apply remaps * refactor: move RelOutputTest to SubstraitRelNodeConverterTest
