Skip to content

Commit

Permalink
optbuilder: disable RANGE window mode with offsets and NULLS LAST
Browse files Browse the repository at this point in the history
This commit makes it so that we return a regular error when building the
window frame with RANGE mode with offsets when NULLS LAST ordering is
requested. The execution engine assumes that exactly one column is
included in the ordering for such a frame, and we recently fixed how we
handle NULLS LAST (by projecting another column), so at the moment with
such queries we encounter a scary-looking internal error, and now we'll
get a regular error. Teaching the execution engine about this is not
exactly trivial, so for now we'll just prohibit such queries.

Epic: None

Release note (bug fix): Previously, CockroachDB could encounter an
internal error when evaluating window functions with RANGE window frame
mode with OFFSET PRECEDING or OFFSET FOLLOWING boundary when ORDER BY
clause has NULLS LAST option. This will now result in a regular error
since the feature is marked as unsupported.
  • Loading branch information
yuzefovich committed Dec 27, 2022
1 parent 0e5acf1 commit 5611e92
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 6 deletions.
8 changes: 8 additions & 0 deletions pkg/sql/opt/optbuilder/testdata/window
Original file line number Diff line number Diff line change
Expand Up @@ -1536,3 +1536,11 @@ sort
│ └── v:3 IS NULL [as=rank_1_nulls_ordering_1_1:8]
└── windows
└── rank [as=rank:7]

# Regression test for an internal error with RANGE mode with offsets and NULLS
# LAST (currently unsupported due to limitations in the execution engine
# #94032).
build
SELECT avg(k) OVER (ORDER BY id NULLS LAST RANGE 0 PRECEDING) FROM nulls_last_test
----
error: NULLS LAST with RANGE mode with OFFSET is currently unsupported
23 changes: 17 additions & 6 deletions pkg/sql/opt/optbuilder/window.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,16 @@ func (b *Builder) buildWindow(outScope *scope, inScope *scope) {
// Build appropriate partitions.
partitions[i] = b.buildWindowPartition(def.Partitions, i, w.def.Name, inScope, argScope)

// Build appropriate orderings.
ord := b.buildWindowOrdering(def.OrderBy, i, w.def.Name, inScope, argScope)
orderings[i].FromOrdering(ord)

var isRangeModeWithOffsets bool
if def.Frame != nil {
windowFrames[i] = *def.Frame
isRangeModeWithOffsets = windowFrames[i].Mode == treewindow.RANGE && def.Frame.Bounds.HasOffset()
}

// Build appropriate orderings.
ord := b.buildWindowOrdering(def.OrderBy, i, w.def.Name, inScope, argScope, isRangeModeWithOffsets)
orderings[i].FromOrdering(ord)

if w.Filter != nil {
col := b.buildFilterCol(w.Filter, i, w.def.Name, inScope, argScope)
filterCols[i] = col.id
Expand Down Expand Up @@ -269,7 +271,7 @@ func (b *Builder) buildAggregationAsWindow(

// Build appropriate orderings.
if !agg.isCommutative() {
ord := b.buildWindowOrdering(agg.OrderBy, i, agg.def.Name, fromScope, g.aggInScope)
ord := b.buildWindowOrdering(agg.OrderBy, i, agg.def.Name, fromScope, g.aggInScope, false /* isRangeModeWithOffsets */)
orderings[i].FromOrdering(ord)
}

Expand Down Expand Up @@ -414,7 +416,11 @@ func (b *Builder) buildWindowPartition(

// buildWindowOrdering builds the appropriate orderings for window functions.
func (b *Builder) buildWindowOrdering(
orderBy tree.OrderBy, windowIndex int, funcName string, inScope, outScope *scope,
orderBy tree.OrderBy,
windowIndex int,
funcName string,
inScope, outScope *scope,
isRangeModeWithOffsets bool,
) opt.Ordering {
ord := make(opt.Ordering, 0, len(orderBy))
for j, t := range orderBy {
Expand All @@ -428,6 +434,11 @@ func (b *Builder) buildWindowOrdering(
expr := tree.NewTypedIsNullExpr(e)
col := outScope.findExistingCol(expr, false /* allowSideEffects */)
if col == nil {
if isRangeModeWithOffsets {
// TODO(yuzefovich): teach the execution engine to
// support this special case.
panic(errors.New("NULLS LAST with RANGE mode with OFFSET is currently unsupported"))
}
// Use an anonymous name because the column cannot be referenced
// in other expressions.
colName := scopeColName("").WithMetadataName(
Expand Down

0 comments on commit 5611e92

Please sign in to comment.