Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

encoding: fix DecodeFloatDescending for positive zero #79473

Merged
merged 1 commit into from
Apr 6, 2022

Conversation

michae2
Copy link
Collaborator

@michae2 michae2 commented Apr 5, 2022

Fixes: #77279

Our old friend -0 is back. This time the problem was in
DecodeFloatDescending, which was decoding both +0 and -0 as -0. Fix it
to decode both as +0. (-0 is then properly decoded as -0 when the value
part of the composite encoding is decoded.)

Note that the on-disk data was correct. Only the decoding was affected.

Release note (bug fix): Queries reading from an index or primary key on
FLOAT or REAL columns DESC would read -0 for every +0 value stored in the
index. Fix this to correctly read +0 for +0 and -0 for -0.

Co-authored-by: Yahor Yuzefovich yahor@cockroachlabs.com

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@michae2 michae2 requested review from yuzefovich, mgartner and a team April 5, 2022 23:56
Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Thanks!

We probably want this to be backported. Thoughts?

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Is TLP failing with this issue on other branches besides master? Seems worthwhile to eliminate the test failures, at least. Otherwise, maybe we should let it bake on master a bit before backporting.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mgartner and @michae2)


pkg/sql/logictest/testdata/logic_test/float, line 84 at r1 (raw file):


query R rowsort
SELECT * FROM i@i_f_idx;

nit: can you give the indexes names above so it's obvious which one you're selecting from?

Fixes: cockroachdb#77279

Our old friend -0 is back. This time the problem was in
DecodeFloatDescending, which was decoding both +0 and -0 as -0. Fix it
to decode both as +0. (-0 is then properly decoded as -0 when the value
part of the composite encoding is decoded.)

Note that the on-disk data was correct. Only the decoding was affected.

Release note (bug fix): Queries reading from an index or primary key on
FLOAT or REAL columns DESC would read -0 for every +0 value stored in the
index. Fix this to correctly read +0 for +0 and -0 for -0.

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
@michae2
Copy link
Collaborator Author

michae2 commented Apr 6, 2022

A few failures were on release-22.1, so definitely worth backporting. How far back do we want to go? Looks like Roachtest Nightly only runs on master and release-22.1?

@rytaft
Copy link
Collaborator

rytaft commented Apr 6, 2022

Doesn't seem like this is a bug that would impact customers, so I'd hesitate to backport to other branches

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

@michae2
Copy link
Collaborator Author

michae2 commented Apr 6, 2022

TFTRs!

bors r=yuzefovich,rytaft,mgartner

@craig
Copy link
Contributor

craig bot commented Apr 6, 2022

Build succeeded:

@craig craig bot merged commit f46a354 into cockroachdb:master Apr 6, 2022
@michae2 michae2 deleted the signedzero branch April 6, 2022 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

roachtest: -0 is not converted to 0, causing spurious tlp failure
5 participants