Skip to content

Commit

Permalink
Merge #90820
Browse files Browse the repository at this point in the history
90820: colexecwindow: fix up some of the gcassert assertions r=yuzefovich a=yuzefovich

This commit removes some of the `gcassert` assertions that are actually
not true - here we use `// gcassert` as opposed to `//gcassert`, and
since we used the old gcassert version, they weren't actually enforced.

Additionally, this commit adjusts the code a bit in order to make the
assertions valid in some cases. One interesting change was to move the
slice accesses into `PerformOperation` which made BCEs to occur.

Epic: None

Release note: None

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
  • Loading branch information
craig[bot] and yuzefovich committed Nov 8, 2022
2 parents 82b2519 + 8091f06 commit 7891d96
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 30 deletions.
9 changes: 0 additions & 9 deletions pkg/sql/colexec/colexecwindow/min_max_queue.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,6 @@ func (q *minMaxQueue) isEmpty() bool {
// get returns the element at position pos in the minMaxQueue (zero-based).
// gcassert:inline
func (q *minMaxQueue) get(pos int) uint32 {
if q.empty || pos < 0 || pos >= q.len() {
colexecerror.InternalError(errors.AssertionFailedf("index out of bounds: %d", pos))
}
return q.buffer[(pos+q.head)%cap(q.buffer)]
}

Expand All @@ -77,17 +74,13 @@ func (q *minMaxQueue) getFirst() uint32 {
// getLast returns the element at the end of the minMaxQueue.
// gcassert:inline
func (q *minMaxQueue) getLast() uint32 {
if q.empty {
colexecerror.InternalError(errors.AssertionFailedf("getting last from empty minMaxQueue"))
}
return q.buffer[(cap(q.buffer)+q.tail-1)%cap(q.buffer)]
}

// addLast adds element to the end of the minMaxQueue and doubles it's
// underlying slice if necessary, subject to the max length limit. If the
// minMaxQueue has already reached the maximum length, addLast returns true,
// otherwise false.
// gcassert:inline
func (q *minMaxQueue) addLast(element uint32) (reachedLimit bool) {
if q.maybeGrow() {
return true
Expand All @@ -99,7 +92,6 @@ func (q *minMaxQueue) addLast(element uint32) (reachedLimit bool) {
}

// removeLast removes a single element from the end of the minMaxQueue.
// gcassert:inline
func (q *minMaxQueue) removeLast() {
if q.empty {
colexecerror.InternalError(errors.AssertionFailedf("removing last from empty ring buffer"))
Expand All @@ -113,7 +105,6 @@ func (q *minMaxQueue) removeLast() {

// removeAllBefore removes from the minMaxQueue all values in the range
// [0, val).
// gcassert:inline
func (q *minMaxQueue) removeAllBefore(val uint32) {
if q.empty {
return
Expand Down
36 changes: 18 additions & 18 deletions pkg/sql/colexec/colexecwindow/min_max_removable_agg.eg.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions pkg/sql/colexec/colexecwindow/min_max_removable_agg_tmpl.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,10 +173,10 @@ func (a *_AGG_TYPEAggregator) processBatch(batch coldata.Batch, startIdx, endIdx
outVec := batch.ColVec(a.outputColIdx)
outNulls := outVec.Nulls()
outCol := outVec.TemplateType()
// {{if not .IsBytesLike}}
_, _ = outCol.Get(startIdx), outCol.Get(endIdx-1)
// {{end}}
a.allocator.PerformOperation([]coldata.Vec{outVec}, func() {
// {{if not .IsBytesLike}}
_, _ = outCol.Get(startIdx), outCol.Get(endIdx-1)
// {{end}}
for i := startIdx; i < endIdx; i++ {
a.framer.next(a.Ctx)
toAdd, toRemove := a.framer.slidingWindowIntervals()
Expand Down

0 comments on commit 7891d96

Please sign in to comment.