Review status: 0 of 2 files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

pkg/sql/row_container.go, line 59 at r1 (raw file):

  // deletedRows is the number of rows that have been deleted from the front
  // of container. When this number reaches rowsPerChunk we delete that chunk

of the container.

pkg/sql/row_container.go, line 60 at r1 (raw file):

  // deletedRows is the number of rows that have been deleted from the front
  // of container. When this number reaches rowsPerChunk we delete that chunk
  // and rest this back to zero.

reset

pkg/sql/row_container.go, line 232 at r1 (raw file):

// Pop 'deletes' the the first rows added to the RowContainer.
func (c *RowContainer) Pop() {

Use PopFirst(), there are other Pop()s which pop from the back (e.g. in heap interface)

pkg/sql/row_container.go, line 233 at r1 (raw file):

// Pop 'deletes' the the first rows added to the RowContainer.
func (c *RowContainer) Pop() {
  c.deletedRows++

check and panic if there are no rows

pkg/sql/row_container.go, line 234 at r1 (raw file):

func (c *RowContainer) Pop() {
  c.deletedRows++
  if c.deletedRows < c.rowsPerChunk {

aren't we supposed to numRows-- ? Please add a check for Len() in the test

pkg/sql/row_container.go, line 235 at r1 (raw file):

  c.deletedRows++
  if c.deletedRows < c.rowsPerChunk {
      return

this return is odd when this is the "common" path and we only have two lines, i'd reverse the condition

pkg/sql/row_container_test.go, line 51 at r1 (raw file):

          }

          rc.Pop()

this probably doesn't test the code that actually removes the chunk. Maybe run more cases , e.g. for _, numPops := range []int{0, 1, 2, numRows/3, numRows/2}

pkg/sql/row_container_test.go, line 54 at r1 (raw file):

          rc.Pop()

          // Given we just deleted two rows, we have numRows - 2 rows to

that

Comments from Reviewable

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

pkg/sql/row_container.go, line 231 at r1 (raw file):

}

// Pop 'deletes' the the first rows added to the RowContainer.

perhaps "discards" would allow you to avoid air quotes.

Comments from Reviewable

Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

pkg/sql/row_container.go, line 59 at r1 (raw file):

pkg/sql/row_container.go, line 60 at r1 (raw file):

pkg/sql/row_container.go, line 231 at r1 (raw file):

pkg/sql/row_container.go, line 232 at r1 (raw file):

pkg/sql/row_container.go, line 233 at r1 (raw file):

pkg/sql/row_container.go, line 234 at r1 (raw file):

pkg/sql/row_container.go, line 235 at r1 (raw file):

pkg/sql/row_container_test.go, line 51 at r1 (raw file):

pkg/sql/row_container_test.go, line 54 at r1 (raw file):

Comments from Reviewable

For posterity, from slack discussion:

PR description currently includes "Fixes #10618" which will cause merging this to close #10618 - that's probably not what you want.

Also, you should keep in mind that preemptive API changes like this (i.e. adding an API in one PR with the intention of using it in the next) is generally a bad idea because it limits the reviewer's ability to help you avoid bad API designs.

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

Comments from Reviewable

ack, TFTRs!

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable