fix(core): require exactly one ordering expression for RANGE window bounds - #1205
fix(core): require exactly one ordering expression for RANGE window bounds#1205anasik wants to merge 2 commits into
Conversation
…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.
dbb53f2 to
ac63942
Compare
alexandrefimov
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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).
BREAKING CHANGE: a
ConsistentPartitionWindoworWindowFunctionInvocationwith aRANGEbound'sPrecedingorFollowingside 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 throwsIllegalArgumentException.Closes #1198