fix(core)!: visit a window relation's input when rewriting it - #1248
Conversation
RelCopyOnWriteVisitor.visit(ConsistentPartitionWindow) rewrote the window functions, the partition expressions and the sorts, and never visited its input. It was the only SingleInputRel in core that did not; Expand and ExtensionWrite throw instead, which is a separate defect. The input is now visited alongside the three lists and folded into the allEmpty check, so a genuinely unchanged relation still comes back as Optional.empty. The existing window tests put a bare namedScan under the relation, which has nothing to rewrite; the new one nests a Project over a literal. Closes substrait-io#1231 BREAKING CHANGE: RelCopyOnWriteVisitor now descends into a ConsistentPartitionWindow's input. A rewrite belonging below a window relation used to be discarded, and a rewrite that applied only there collapsed the whole traversal to Optional.empty; both now give back the window relation with its input rewritten.
visit(MultiBucketExchange) computed the rewritten expression and then decided "nothing changed" from the input alone, so an expression-only rewrite was discarded the same way the window relation's input was. visit(SingleBucketExchange) already had the form. The test fails without it, and the window Javadoc now says what the test asserts rather than what the visitor used to do.
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Add a case to OuterReferenceConverterTest for a correlated subquery under a window relation — that is the public-API bug this actually fixes, and a round trip cannot pin it because both directions were no-ops before, so assert on the id-based plan directly. Separately, consider dropping the ! and the footer: #1136 was authored that way and merged as plain fix(core):. If you keep it, the footer should mention that descending now surfaces the #1185 UnsupportedOperationException for an Expand or DDL relation under a window.
Drives the check from the shared samples instead of one hand-picked relation, so a relation added without visiting its input fails here too rather than dropping rewrites silently. On fb6a54a it names ConsistentPartitionWindow. Expand and ExtensionWrite are exempt because the visitor refuses them outright rather than descending into them. The three window fixtures in the file differed only in the input and the bounds, so they collapse into one windowOver helper, which takes the no-op empty argument list with them.
A correlated subquery in a window relation's input was invisible to OuterReferenceConverter in both directions, so a round trip through it was an identity and could not tell the fix from the bug. The case asserts the anchor and the rel_reference on the id-based plan instead, and fails on fb6a54a.
|
Added Kept the The editorconfig job is red on its own download — |
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
LGTM, #1272 will fix the editorconfig check
The rename-a-scan sweep exercises only each guard's input branch, so it would not have caught the MultiBucketExchange half of this PR: on the base visitor it reports ConsistentPartitionWindow and the exchange passes. A second sweep marks every field reference instead and pins the relations whose samples carry one, so a relation whose visit computes an expression and then decides from its input alone drops out of that set. On the base MultiBucketExchange is the one missing. negateI32LiteralsVisitor could not do it. No sample carries an i32 literal, and widening it to i64 reaches only Fetch, Filter and TopN -- not the exchange, whose sample expression is a field reference.
2 participants
RelCopyOnWriteVisitor.visit(ConsistentPartitionWindow)rewrote the window functions, the partition expressions and the sorts, but never visited the relation's input. A rewrite that belonged below a window was therefore dropped, and one that applied nowhere else made the whole traversal come back asOptional.empty()— the visitor's way of saying nothing changed. It visits the input now and counts it in that decision.visit(MultiBucketExchange)computed a rewritten expression and then left it out of the same decision, so it counts that too.OuterReferenceConverteris built on this visitor, which is where the defect was reachable from outside: a correlated subquery in a window's input never got to it, so neither direction of the offset-based/id-based conversion touched the reference. The new tests pin that, and drive the input check from the shared samples so a relation added without visiting its input fails too — on the base that check namesConsistentPartitionWindow.Closes #1231
BREAKING CHANGE: a plan carrying an
Expand, anExtensionWrite, aNamedDdlor anExtensionDdlbelow aConsistentPartitionWindownow throwsUnsupportedOperationExceptionfromio.substrait.relation.RelCopyOnWriteVisitor, which used to stop above those relations and return quietly. Those four visits refuse rather than descend, so a consumer needing such a plan rewritten has to override them;OuterReferenceConverterruns this visitor, which makes the throw reachable without naming the class. Rewrites that belong below a window relation, or that touch only aMultiBucketExchange's expression, now take effect instead of being discarded.