sql/parser: Verify that AggregateFuncs deep copy Datums on Result

[nvanbenschoten]

Requested as a separate PR from #8928.

This makes sure that all Datums returned from the Result method of
AggregateFunc implementations are deep copies of any internal state.
This is necessary to allow for ordered window functions that will
incrementally call Result on an aggregate, and expect each resultant
Datum to be a separate deep copy and not modified during future
accumulation.

---

This change is

[knz]

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

---

sql/parser/aggregate_builtins.go, line 43 [r1] (raw file):

  // Result returns the current value of the accumulation. This value
  // will be a deep copy of any AggregateFunc internal state, so that
  // it will not be mutated by additional calls to Add.

Do you need to be able to extract the result and also be able to continue aggregating afterwards?
If not then I don't like this definition of the interface. In that case what about: "This returns the result of the accomulation, then resets the internal state so the result does not get updated by subsequent calls to Add."
See my other comments too.

---

sql/parser/aggregate_builtins.go, line 451 [r1] (raw file):

      return DNull
  }
  dd := &DDecimal{}

If reading the results amounts to a reset I'd have the decSum be a *DDecimal, then:

res := a.decSum
a.decSum := &DDecimal{}
return res

(We want to optimize the common case. )

---

Comments from Reviewable

[nvanbenschoten]

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

---

sql/parser/aggregate_builtins.go, line 43 [r1] (raw file):

Previously, knz (kena) wrote…

Do you need to be able to extract the result and also be able to continue aggregating afterwards?
If not then I don't like this definition of the interface. In that case what about: "This returns the result of the accomulation, then resets the internal state so the result does not get updated by subsequent calls to Add."
See my other comments too.

Yeah the whole point is that we need to be able to extract the result incrementally as we accumulate without future aggregation modifying the `Datum` returned. We specifically **do not** want to reset the internal state.

An example of why this is important is shown by the query SELECT salary, sum(salary) OVER (ORDER BY salary) FROM empsalary seen here. Each row in the second column of that query (and in any other aggregate ordered window function) will come from an incremental call to intSumAggregate.Result(), so we do not want to reset on Result.

---

Comments from Reviewable

[knz]

LGTM