Skip to content

core: RelCopyOnWriteVisitor throws for Expand, ExtensionWrite, NamedDdl and ExtensionDdl, so rewriting a plan holding one fails #1185

Description

@nielspardon

RelCopyOnWriteVisitor rebuilds a relation when something below it changed and returns Optional.empty() when nothing did, for every relation except four, which throw a bare UnsupportedOperationException() with no message: Expand, ExtensionWrite, NamedDdl and ExtensionDdl.

https://github.com/substrait-io/substrait-java/blob/main/core/src/main/java/io/substrait/relation/RelCopyOnWriteVisitor.java#L275-L306

The gap is already worked around once in main source, which is the clearest statement of what the base class should do. OuterReferenceConverter.RelRewriter overrides visit(NamedDdl) with the comment "The copy-on-write base throws for DDL; a view definition is an independent (top-level) query that may itself contain correlated subqueries, so traverse it rather than throw", and then does exactly that traversal.

Where it is not worked around it fails. SchemaCollector.TableGatherer overrides visit(NamedScan) and visit(NamedWrite) but not visit(NamedDdl), so SubstraitToCalcite.convert(Rel) without a catalog reader — the path that resolves the schema by walking the plan — throws the bare exception on any plan holding a CREATE VIEW, naming neither the relation nor the reason.

The asymmetry inside the class is the part worth closing first: visit(NamedWrite) rebuilds normally while visit(NamedDdl) throws, even though a NamedDdl's view_definition is a Rel in exactly the way a NamedWrite's input is. That matters more now that #1181 gives the DDL relations a declared schema to carry, since a consumer using this visitor to rewrite a plan is the case the schema exists for.

Each of the four can rebuild the way its siblings do — NamedDdl by traversing view_definition as RelRewriter already shows, ExtensionWrite on the model of NamedWrite, Expand through its input and its fields, ExtensionDdl through its own definition. Where one genuinely cannot be rewritten, the throw should at least name the relation it refused.

Found while reviewing #1181, whose new view round-trip test carries a comment explaining that it cannot use assertFullRoundTrip for this reason.

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