Thanks for taking this on!

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mneverov)

-- commits, line 2 at r1:
nit: use builtins: as the prefix of the commit title and use the lower case for Used.

-- commits, line 17 at r1:
nit: missing Release note: None.

pkg/sql/sem/builtins/aggregate_builtins.go, line 1849 at r1 (raw file):

}

func newRegressionAccumulatorDecimalBase(

super nit: there is a slight difference between new and make for the constructor methods naming (at least in CockroachDB code). new is used for pointer returns whereas make for value returns, so we should use make here and for Empty below as well.

pkg/sql/sem/builtins/aggregate_builtins.go, line 1866 at r1 (raw file):

		singleDatumAggregateBase: singleDatumAggregateBase,
		ed:                       ed,
		n:                        &apd.Decimal{},

Not sure if it matters, but it might be better to have a single large allocation, so something like

var decimals [16]apd.Decimal
b := regressionAccumulatorDecimalBase{
  ...
  n: &decimals[0],
  sx: &decimals[1],
  ...
}

Or maybe the compiler is smart enough to do something like this itself.

In any case, it'd be good to take a quick look at the benchmarks.

pkg/sql/sem/builtins/aggregate_builtins.go, line 1979 at r1 (raw file):

		// sums to NaN; otherwise we will falsely report variance zero when
		// there are no more inputs.
		if x.Form == apd.NaN || isInf(x) {

super nit: might be cleaner to introduce isNaN helper method as well.

pkg/sql/sem/builtins/aggregate_builtins.go, line 2034 at r1 (raw file):

func mapToDFloat(d *apd.Decimal, err error) (tree.Datum, error) {
	if err != nil {
		return tree.DNull, nil

Shouldn't this return err?

Update: our linter caught it too :)

        pkg/sql/sem/builtins/aggregate_builtins.go:2034:3: unexpected nil error return after checking for a non-nil error; if this is not a mistake, add a "//nolint:returnerrcheck" comment

pkg/sql/sem/builtins/aggregate_builtins.go, line 2039 at r1 (raw file):

	res, err := d.Float64()

	if err != nil && err.(*strconv.NumError).Err == strconv.ErrRange {

nit: can we replace the second part of the conditional with something like errors.Is(err, strconv.ErrRange)?

Update: linter is suggesting this as well :)

        pkg/sql/sem/builtins/aggregate_builtins.go:2039:47: use errors.Is instead of a direct comparison
        For example:
           if errors.Is(err, errMyOwnErrReference) {
             ...
           }
        
        pkg/sql/sem/builtins/aggregate_builtins.go:2039:23: invalid direct cast on error object
        Alternatives:
           if _, ok := err.(*T); ok        ->   if errors.HasType(err, (*T)(nil))
           if _, ok := err.(I); ok         ->   if errors.HasInterface(err, (*I)(nil))
           if myErr, ok := err.(*T); ok    ->   if myErr := (*T)(nil); errors.As(err, &myErr)
           if myErr, ok := err.(I); ok     ->   if myErr := (I)(nil); errors.As(err, &myErr)
           switch err.(type) { case *T:... ->   switch { case errors.HasType(err, (*T)(nil)): ...

pkg/sql/sem/builtins/aggregate_builtins.go, line 2074 at r1 (raw file):

		return tree.DNull, nil
	}
	res := &apd.Decimal{}

IIUC we could avoid the allocation of a new decimal here and reuse a.tmp, right?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mneverov and @yuzefovich)

-- commits, line 17 at r1:

👍

pkg/sql/sem/builtins/aggregate_builtins.go, line 1849 at r1 (raw file):

OK

pkg/sql/sem/builtins/aggregate_builtins.go, line 1979 at r1 (raw file):

Done.

pkg/sql/sem/builtins/aggregate_builtins.go, line 2034 at r1 (raw file):

fixed, thanks

pkg/sql/sem/builtins/aggregate_builtins.go, line 2039 at r1 (raw file):

Done.

pkg/sql/sem/builtins/aggregate_builtins.go, line 2074 at r1 (raw file):

you're right, replaced where possible.

Last question from me, but otherwise

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mneverov)

pkg/sql/sem/builtins/aggregate_builtins.go, line 1842 at r2 (raw file):

	// Variables used across iterations.
	ed                       *apd.ErrDecimal
	n, sx, sxx, sy, syy, sxy *apd.Decimal

Actually, rather than storing decimals by pointer, can we store them by value here? Then we won't need to explicitly set them in the constructor.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)

pkg/sql/sem/builtins/aggregate_builtins.go, line 1842 at r2 (raw file):

Yes we can. I decided against it because then in the add funcion and in all result functions I would need to use & operator.
Don't have strong opinion about it, let me know if the values in the struct are preferable.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mneverov)

pkg/sql/sem/builtins/aggregate_builtins.go, line 1842 at r2 (raw file):

I think the values are preferable in this case. My reasoning is that then all apd.Decimal objects don't have to be explicitly instantiated and all of them are also in the same memory location as regressionAccumulatorDecimalBase struct as a whole. Taking the pointer from value (i.e. & operator) is essentially free AFAIK.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @yuzefovich)

pkg/sql/sem/builtins/aggregate_builtins.go, line 1842 at r2 (raw file):

Sure, & is free, I was thinking from the code aesthetics perspective :)

bors r=yuzefovich

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mneverov)

pkg/sql/sem/builtins/aggregate_builtins.go, line 1842 at r2 (raw file):

I see :)