Skip to content

isthmus: nothing fails when a relation's conversion forgets applyRelCommon, so its emit mapping is dropped again #1215

Description

@nielspardon

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions