Every relation's conversion has to remember to call applyRelCommon, and nothing fails when one does not:
|
* |
|
* @param relNode the node the relation was converted into |
|
* @param rel the relation being converted |
|
* @param inputs the nodes this relation's inputs were converted into |
|
* @return the node, remapped and renamed |
|
*/ |
|
protected RelNode applyRelCommon(RelNode relNode, Rel rel, RelNode... inputs) { |
|
return applyOutputNames(applyRemap(relNode, rel.getRemap()), rel, inputs); |
|
} |
#1160 was exactly that omission, in the one visit that never called it, found one commit after the helper was introduced.
core prevents the analogous mistake structurally: core/src/test/java/io/substrait/utils/RelSamples.java plus RelSamplesTest reflect over RelVisitor's visit overloads so a relation with no sample fails the build, and RelCommonRoundtripTest then round-trips RelCommon for every one of them. isthmus has no equivalent -- coverage is hand-picked per relation -- and isthmus/build.gradle.kts does not depend on core's test output, so the fixture has to be isthmus-side rather than reused.
What would hold the invariant: for every RelVisitor.visit overload this converter implements, convert a sample carrying a Rel.Remap and assert the result either has the remapped row type or refuses. The relations that reach visitFallback refuse everything already, so they pass for free, and the next relation wired in fails until its mapping is routed.
Related to #1123, which folds a hand-rolled per-relation sample set onto the core fixture.
Found while reviewing #1189.
Every relation's conversion has to remember to call
applyRelCommon, and nothing fails when one does not:substrait-java/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java
Lines 1170 to 1178 in 934a60e
#1160 was exactly that omission, in the one
visitthat never called it, found one commit after the helper was introduced.core prevents the analogous mistake structurally:
core/src/test/java/io/substrait/utils/RelSamples.javaplusRelSamplesTestreflect overRelVisitor'svisitoverloads so a relation with no sample fails the build, andRelCommonRoundtripTestthen round-tripsRelCommonfor every one of them. isthmus has no equivalent -- coverage is hand-picked per relation -- andisthmus/build.gradle.ktsdoes not depend on core's test output, so the fixture has to be isthmus-side rather than reused.What would hold the invariant: for every
RelVisitor.visitoverload this converter implements, convert a sample carrying aRel.Remapand assert the result either has the remapped row type or refuses. The relations that reachvisitFallbackrefuse everything already, so they pass for free, and the next relation wired in fails until its mapping is routed.Related to #1123, which folds a hand-rolled per-relation sample set onto the core fixture.
Found while reviewing #1189.