builtins: fix incorrect sqrdiff evaluation due to reuse of the results from previous iterations when used as a window function by mneverov · Pull Request #55957 · cockroachdb/cockroach

mneverov · 2020-10-25T11:55:59Z

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 · 2020-10-25T11:56:04Z

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 · 2020-10-25T11:56:07Z

This change is

blathers-crl · 2020-10-25T17:46:57Z

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 · 2020-10-25T18:40:35Z

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 · 2020-10-25T19:59:10Z

@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 · 2020-10-26T16:53:58Z

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 · 2020-10-26T16:58:40Z

@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.

…s from previous iterations when used as a window function fixes cockroachdb#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.

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 · 2020-10-26T18:12:39Z

Build succeeded:

GitHub CI (Cockroach)

blathers-crl bot added O-community Originated from the community X-blathers-untriaged blathers was unable to find an owner labels Oct 25, 2020

mneverov changed the title ~~builtins: fix aggregate functions for decimals~~ WIP: builtins: fix aggregate functions for decimals Oct 25, 2020

mneverov force-pushed the windowed_aggregate_fix branch from 8be3cec to afc91f7 Compare October 25, 2020 18:40

mneverov changed the title ~~WIP: builtins: fix aggregate functions for decimals~~ builtins: fix aggregate functions for decimals Oct 25, 2020

mneverov marked this pull request as ready for review October 25, 2020 19:59

yuzefovich requested review from a team and yuzefovich October 26, 2020 15:20

yuzefovich approved these changes Oct 26, 2020

View reviewed changes

mneverov force-pushed the windowed_aggregate_fix branch from afc91f7 to f6eefbe Compare October 26, 2020 16:53

mneverov commented Oct 26, 2020

View reviewed changes

mneverov changed the title ~~builtins: fix aggregate functions for decimals~~ builtins: fix incorrect sqrdiff evaluation due to reuse of the results from previous iterations when used as a window function Oct 26, 2020

yuzefovich reviewed Oct 26, 2020

View reviewed changes

mneverov force-pushed the windowed_aggregate_fix branch from f6eefbe to 4bf1804 Compare October 26, 2020 17:06

mneverov commented Oct 26, 2020

View reviewed changes

yuzefovich reviewed Oct 26, 2020

View reviewed changes

craig bot merged commit 4a58b36 into cockroachdb:master Oct 26, 2020

This was referenced Oct 26, 2020

release-20.2: builtins: fix sqrdiff when used as a window function #55995

Merged

release-20.1: builtins: fix sqrdiff when used as a window function #55999

Merged

Conversation

mneverov commented Oct 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blathers-crl bot commented Oct 25, 2020

Uh oh!

cockroach-teamcity commented Oct 25, 2020

Uh oh!

blathers-crl bot commented Oct 25, 2020

Uh oh!

blathers-crl bot commented Oct 25, 2020

Uh oh!

mneverov commented Oct 25, 2020

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

blathers-crl bot commented Oct 26, 2020

Uh oh!

mneverov left a comment

Choose a reason for hiding this comment

Uh oh!

mneverov commented Oct 26, 2020

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

mneverov left a comment

Choose a reason for hiding this comment

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

craig bot commented Oct 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mneverov commented Oct 25, 2020 •

edited

Loading