Skip to content

encoding: fix DecodeFloatDescending for positive zero#79473

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
michae2:signedzero
Apr 6, 2022
Merged

encoding: fix DecodeFloatDescending for positive zero#79473
craig[bot] merged 1 commit intocockroachdb:masterfrom
michae2:signedzero

Conversation

@michae2
Copy link
Copy Markdown
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
Copy Markdown
Member

This change is Reviewable

@michae2 michae2 requested review from a team, mgartner and yuzefovich April 5, 2022 23:56
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

@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
Copy Markdown
Collaborator Author

michae2 commented Apr 6, 2022

TFTRs!

bors r=yuzefovich,rytaft,mgartner

@craig
Copy link
Copy Markdown
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