builtins: fix incorrect sqrdiff evaluation due to reuse of the results from previous iterations when used as a window function

[mneverov]

builtins: fix incorrect sqrdiff evaluation due to reuse of the results from previous iterations when used as a window function

fixes #55944

Release note (bug fix): CockroachDB previously could incorrectly evaluate sqrdiff function when used as a window function in some cases, and now it is fixed.

[blathers-crl]

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

• A person you worked with closely on this PR.
• The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
• Join our community slack channel and ask on #contributors.
• Try find someone else from here.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[cockroach-teamcity]

This change is 

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• When CI has completed, please ensure no errors have appeared.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

• A person you worked with closely on this PR.
• The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
• Join our community slack channel and ask on #contributors.
• Try find someone else from here.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[blathers-crl]

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

• A person you worked with closely on this PR.
• The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
• Join our community slack channel and ask on #contributors.
• Try find someone else from here.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[mneverov]

@yuzefovich could you please take a look?
The root cause was that the result of the sqrdiff function was reused: tree.DDecimal{Decimal: a.sqrDiff} copies the pointer to the underlying slice of the a.sqrDiff.
Order within windowed aggregate functions does not affect order of the output when run on several nodes, i.e. the order of rows when running select sqrdiff(x) OVER (ORDER BY x) from t is unpredictable. As I understand from the other tests in this file this works as intended so I added the column x as well as order by x to the sql statement.

[yuzefovich]

Thanks for finding the bug and fixing it! I only have some nits, but it

Re: ordering: yeah, ORDER BY within OVER() clause doesn't guarantee the ordering of the results, we need to have a query level ORDER BY clause for that.

nit: it'd be nice to include a short description of the bug into the commit message, and we also should add a release note (it'll be something like Release note (bug fix): CockroachDB previously could incorrectly evaluate sqrdiff function when used as a window function in some cases, and now it is fixed.).

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mneverov)

---

pkg/sql/logictest/testdata/logic_test/aggregate, line 2862 at r1 (raw file):

0.522232967867094 3

# Regression test for window aggregate functions for decimals reuse the results

nit: this test should go into window logic test file.

nit: I'd slightly reword the comment to be a bit more specific - Regression test for window aggregate functions not making a deep copy of decimals on each iteration (#55944).

[blathers-crl]

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[mneverov]

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

---

pkg/sql/logictest/testdata/logic_test/aggregate, line 2862 at r1 (raw file):

Previously, yuzefovich wrote…

nit: this test should go into window logic test file.

nit: I'd slightly reword the comment to be a bit more specific - Regression test for window aggregate functions not making a deep copy of decimals on each iteration (#55944).

moved to window, replaced the comment, thanks!

[mneverov]

@yuzefovich thanks for the review.
Could you please check if the commit message contains enough / proper info?

[yuzefovich]

Thanks, one last nit.

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mneverov)

---

pkg/sql/logictest/testdata/logic_test/window, line 3866 at r2 (raw file):

{"1": "1", "2": "2", "3": "3", "4": "4", "5": "5"}

Regression test for window aggregate functions not making a deep copy of decimals on each iteration (#55944)

nit: missing # in the beginning and a period in the end.

[mneverov]

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

---

pkg/sql/logictest/testdata/logic_test/window, line 3866 at r2 (raw file):

Previously, yuzefovich wrote…

nit: missing # in the beginning and a period in the end.

fixed thx

[yuzefovich]

Thanks!

bors r=yuzefovich

Reviewed 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

[craig]

Build succeeded:

• GitHub CI (Cockroach)