Skip to content

fix(core): require exactly one ordering expression for RANGE window bounds - #1205

Open
anasik wants to merge 2 commits into
substrait-io:mainfrom
anasik:core-range-window-ordering
Open

fix(core): require exactly one ordering expression for RANGE window bounds#1205
anasik wants to merge 2 commits into
substrait-io:mainfrom
anasik:core-range-window-ordering

Conversation

@anasik

@anasik anasik commented Aug 31, 2026

Copy link
Copy Markdown
Contributor
  • Requires exactly one, non-CLUSTERED ordering expression for a RANGE bound with a Preceding/Following side
  • Fixed 4 pre-existing test fixtures that were building this invalid shape

BREAKING CHANGE: a ConsistentPartitionWindow or WindowFunctionInvocation with a RANGE bound's Preceding or Following side now requires exactly one, non-CLUSTERED ordering expression. A plan that previously built or parsed with zero, multiple, or a CLUSTERED ordering expression in that position now throws IllegalArgumentException.

Closes #1198

@anasik anasik changed the title fix(core): require exactly one ordering expression for RANGE window b… fix(core): require exactly one ordering expression for RANGE window bounds Aug 31, 2026
…bounds

BREAKING CHANGE: a ConsistentPartitionWindow or WindowFunctionInvocation
with a RANGE bound's Preceding or Following side now requires exactly
one, non-CLUSTERED ordering expression. A plan that previously built or
parsed with zero, multiple, or a CLUSTERED ordering expression in that
position now throws IllegalArgumentException.
@anasik
anasik force-pushed the core-range-window-ordering branch from dbb53f2 to ac63942 Compare August 31, 2026 13:24

@alexandrefimov alexandrefimov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the three rules against algebra.proto at v0.102.0, the version the catalog pins: the bounds_type comment on WindowRelFunction carries both halves for the relation, and for the expression form the "exactly one ordering expression" half lives on the BOUNDS_TYPE_RANGE enum, which both kinds share. So both checks land where the spec puts them.

Three things I would add, none of them blocking.

The expression-side check has no test behind it. Neutering checkRangeOrdering and running :core:test fails exactly three of 743 tests — the three new ones, all on ConsistentPartitionWindow. Expression.WindowFunctionInvocation.check() gained the same call, and the fixture in windowFunctionInvocationRoundtripWithNonLiteralOffsetExpr needed its sort precisely because of it, but nothing pins the rejection there. The two checks sit in different classes and can regress independently, so one mirror of rangePrecedingWithTwoOrderingExpressionsIsRejected on the invocation would cover it.

On the issue's open question about CLUSTERED reachability: isthmus cannot emit it on a window — WindowFunctionConverter maps a window's collations through SortFieldConverter.asSortDirection, which throws for any direction but ASCENDING and DESCENDING. The CLUSTERED mapping isthmus does have, SubstraitRelVisitor.asSortDirection, is on the SortRel path. The one producer in the repo that builds a ConsistentPartitionWindow from user input is Spark's visitWindow, which takes its sorts straight from window.orderSpec. Worth a line in the description naming what this can newly reject, since the BREAKING CHANGE note reads as if any producer might be affected.

The third rule in that paragraph cannot be checked at all: Expression.SortField models only expr() and direction(), so a custom comparison function has no POJO representation to reject. The issue says so; the description does not, and the paragraph reads as fully enforced without it.

@nielspardon nielspardon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a sorts-carrying overload to SubstraitBuilder.windowFn — it never sets sort(), so after this change every RANGE frame with a Preceding/Following bound built through the DSL throws with no way for the caller to supply the ordering the check now demands (sb.windowFn(FUNCTIONS_ARITHMETIC, "lead:any", R.I64, INITIAL_TO_RESULT, ALL, RANGE, WindowBound.Preceding.of(5), WindowBound.CURRENT_ROW)...requires exactly one ordering expression, but found 0); ExpressionCreator.windowFunction already takes a sort list. Also retitle to fix(core)!: — the squash-merge message comes from the PR title, so without the ! the CHANGELOG entry carries no breaking marker.

Separately from the comparison-function clause already raised above: #1198's offset/ordering type-compatibility rule (add(T, D) -> T) is untouched, and that issue's own open question asked for a decision on it. Is Closes #1198 intended here, or should that rule get a follow-up issue first?

.sorts(
Arrays.asList(
Expression.SortField.builder()
.expr(sb.fieldReference(input, 0))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make the ordering column R.I32 in the namedScan on line 44 so it matches the i32 offsets. As added, this fixture pairs an i64 ordering expression with Preceding.of(sb.i32(5))/Following.of(sb.i32(7)), and add(i64, i32) -> i64 is not a declared arithmetic variant — so the fixture now encodes exactly the offset/ordering rule this PR leaves unchecked, and will need fixing again when that rule lands. Before this change these fixtures had no ordering expression at all, so no mismatch existed; same shape at ExpressionCopyOnWriteVisitorTest.java:102, where the sort expr is sb.i64(1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

core: RANGE window frames accept sorts and offset types the spec disallows

3 participants