refactor(isthmus)!: remove dead WindowRelFunctionConverter and de-duplicate call converters

[nielspardon]

What

P0 (debris cleanup) of the Isthmus function-mapping refactor:

1. Delete the dead WindowRelFunctionConverter. It was never instantiated or wired into any ConverterProvider, and it duplicated WindowFunctionConverter's signature matching against SimpleExtension.WindowFunctionVariant. What it produced — ConsistentPartitionWindow.WindowRelFunctionInvocation — is built only in :core (proto read and copy-on-write rewrite) and by the :spark integration's own converter, both untouched.
2. De-duplicate the call-converter list. ConverterProvider.getCallConverters() now builds on CallConverters.defaults(typeConverter) (previously it re-listed the identical first eight converters) and then appends CREATE_SEARCH_CONV and the scalar function converter, as before. Order, instances, and the mutable-ArrayList return type are unchanged, so subclasses that mutate super.getCallConverters() (e.g. DynamicConverterProvider) keep working.
3. Fix stale javadoc in SimpleExtensionToSqlOperator: it already handles window functions via toWindowSqlFunction, but the class and its ExtensionCollection overloads still said "scalar and aggregate … window functions are not yet implemented".

What is being dropped

WindowRelFunctionConverter was the expression half of a Calcite LogicalWindow → Substrait ConsistentPartitionWindow path whose rel half was never written. Nothing in Isthmus constructs a ConsistentPartitionWindow; the relation appears there only in SqlKindFromRel, which reads Substrait relations rather than producing them. So this removes the unreachable half of a conversion path that never existed, rather than closing off a working one. Building that path out properly is tracked in #1075, which also covers the missing SubstraitRelNodeConverter.visit(ConsistentPartitionWindow); git history keeps the prior attempt recoverable in the meantime.

Why

First and lowest-risk step of the larger function-mapping redesign.

Part of #1012 (P0); part of epic #1013.

🤖 Generated with AI

BREAKING CHANGE: the public io.substrait.isthmus.expression.WindowRelFunctionConverter is removed. No shipped ConverterProvider ever instantiated it, so no configured conversion path reached it, but it was public API with public constructors and a public convert(...), so a downstream that constructed it directly will no longer compile. There is no drop-in replacement: WindowFunctionConverter performs the same signature matching against SimpleExtension.WindowFunctionVariant, but yields an Expression.WindowFunctionInvocation rather than a ConsistentPartitionWindow.WindowRelFunctionInvocation.

[alexandrefimov]

Checked the mechanical parts against main and they hold up:

• WindowRelFunctionConverter has no references anywhere in the tree except one line in CHANGELOG.md.
• CallConverters.defaults(typeConverter) returns the same eight converters in the same order as the inlined list, the result stays a mutable ArrayList, and DynamicConverterProvider.getCallConverters() still appends to super.getCallConverters() unaffected.
• The SimpleExtensionToSqlOperator javadoc fix is correct — from(...) flat-maps collection.windowFunctions() and toSqlFunction dispatches to toWindowSqlFunction, so the "not yet implemented" note was stale.

Two things worth settling before this lands.

Should this be refactor(isthmus)!? WindowRelFunctionConverter is public, with public constructors and a public convert(...), in the published :isthmus artifact. It is unused in-tree, but it is also self-contained and does something useful on its own, so a downstream that instantiated it directly stops compiling. Comparable isthmus removals were marked breaking (#998, #1035); the title here carries no ! and the description no BREAKING CHANGE: footer, so semantic-release would cut this without flagging the removal.

Worth naming what is being dropped. This is not only dead code — it is the expression half of a Calcite LogicalWindow → Substrait ConsistentPartitionWindow path whose rel half was never written (ConsistentPartitionWindow appears in isthmus only in SqlKindFromRel, i.e. the other direction). Removing it is reasonable and git history keeps it recoverable, but a line in the description saying so would save the next person from rediscovering the gap.

Also needs a rebase — ConverterProvider has moved since mid-July (builder, casing, type observer), and the PR currently shows conflicts.

[nielspardon]

Both points taken — applied.

Marked breaking. Retitled refactor(isthmus)! with a BREAKING CHANGE: footer. You're right that the removal is API-visible: the class is public with two public constructors and a public convert(...), and it shipped as a feature in #234. #1006 (refactor(core)!: remove deprecated Join.JoinType.SEMI/ANTI) is the closest precedent — same type, same shape — so this is consistent with it.

One thing worth flagging from that precedent: #1006 put its rationale under a ## Breaking change markdown heading rather than a BREAKING CHANGE: footer, so its release-notes entry rendered as just the bare commit subject and the "use LEFT_SEMI/LEFT_ANTI instead" guidance never reached the changelog. #1035, which used a real footer, rendered in full. So this one uses an actual footer, placed last.

For the record on version impact: .releaserc.mjs sets releaseRules: [{ breaking: true, release: "minor" }], so the ! lands this as 0.99.0 rather than forcing 1.0.0.

Named what's dropped. Added a "What is being dropped" section. Your read matches what I found — ConsistentPartitionWindow appears in Isthmus only in SqlKindFromRel, which reads Substrait relations rather than producing them, so the rel half of the Calcite LogicalWindow → ConsistentPartitionWindow path genuinely was never written.

I also made the footer explicit that there is no drop-in replacement, which is a wrinkle worth naming: WindowFunctionConverter duplicates the signature matching against SimpleExtension.WindowFunctionVariant, but its third type parameter is Expression.WindowFunctionInvocation, not ConsistentPartitionWindow.WindowRelFunctionInvocation. So the two are duplicative in matching logic but not interchangeable in output.

Rebase. Merged main in rather than rebasing, to keep the review history intact. The only conflict was the ConverterProvider import block: main added TypeObserver (for the new getTypeObserver()) and still listed SqlArrayValueConstructorCallConverter / SqlMapValueConstructorCallConverter, which this PR drops as part of the CallConverters.defaults() change. Kept TypeObserver, dropped the other two. getCallConverters() itself merged clean — main never touched it — and I re-confirmed on merged main that CallConverters.defaults() still returns the same eight converters in the same order.

[nielspardon]

Filed #1075 for the window-relation gap and linked it from the description, so the removal points at where the real work is tracked rather than leaving it to be rediscovered.

Worth recording one finding from scoping it, since it bears on whether this deletion was the right call: WindowRelFunctionConverter would not have been a usable starting point for that work. WrappedWindowRelCall.getOperands() passed the RexWinAggCall operands through raw, with no resolution against Window.constants — and Calcite hoists literal operands into constants, rewriting them as RexInputRefs with ordinals past the input field width. So NTILE(4), LAG(x, 2) and LEAD(x, 1, 0) would have converted those literals as field references: silently wrong plans rather than an exception. It also cannot be fixed in place, because convert(...) never receives the enclosing Window and so structurally cannot resolve them.

The issue also captures the level mismatch in both directions (Substrait keeps partitions/sorts on the relation while Calcite's Window.Group carries keys, order keys and bounds together), the lack of any Substrait counterpart for Window.Group.exclude, and a recommendation to target the expression form first — it reuses the already-wired WindowFunctionConverter and round-trips today, whereas emitting ConsistentPartitionWindow would produce plans Isthmus cannot read back.

[alexandrefimov]

Verified CallConverters.defaults() returns the same eight converters in the same order, and getCallConverters() still hands back a mutable ArrayList, so DynamicConverterProvider is unaffected.