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

sqllogictest: treat -0 as 0 #7096

Conversation

philip-stoev
Copy link
Contributor

For some reason, slt_good_0.test:51762 returns -0 under a coverage
build and 0 otherwise. To make the test pass in both environments,
have the test driver remove the minus sign from any -0 values.


I am proposing this change because of the following two things:

  1. The coverage build fails like this:
==> test/sqllogictest/sqlite/test/random/select/slt_good_0.test
    SELECT + col0 / - CAST ( 32 AS REAL ) FROM tab2 AS cor0
    OutputFailure:test/sqllogictest/sqlite/test/random/select/slt_good_0.test:51762
            expected: Values(["-2", "-2", "0"])
            actually: Values(["-0", "-2", "-2"])
            actual raw: [Row { columns: [Column { name: "?column?", type: Float4 }] }, Row { columns: [Column { name: "?column?", type: Float4 }] }, Row { columns: [Column { name: "?column?", type: Float4 }] }]

In other words -0 is returned where 0 was previously expected.

The normal build is not affected. This may be a bug in the compiler or the coverage instrumentation.

  1. With the APD type going forward I do not think it will be possible for us to return 0 or -0 exactly the way Postgres does it. So ignoring the minus sign could potentially allow us to bring more SLT tests to bear.

For some reason, slt_good_0.test:51762 returns -0 under a coverage
build and 0 otherwise. To make the test pass in both environments,
have the test driver remove the minus sign from any -0 values.
@sploiselle
Copy link
Contributor

I'm not very familiar with the inner-workings of sqllogictest, but this approach strikes me as a little odd––though quite possible that it doesn't matter in the context of the tool. My inclination would be to massage the values that get string-ified rather than trying to catch individual strings. e.g. what about "0.0"? Is that valid output, or is there simply nothing expressing that value in the test cases yet?

As for APD, that is a replacement only for the decimal type, which doesn't support outputting negative 0 (though negative zero is valid input; its sign is simply flipped to a positive. That is to say that hopefully this change won't affect APD, though if we did inadvertently leak negative zero values, I'd prefer sqllogictest make it possible to catch them.

@benesch
Copy link
Member

benesch commented Jun 18, 2021

Yeah, this seems very scary to me. Our coverage builds are resulting in different results than the normal builds?! Yeesh.

@benesch
Copy link
Member

benesch commented Jun 18, 2021

I actually think this is just a bug in how we format floats as integers in the SLT runner. Probably shouldn't be outputting -0 ever as that's not an integer.

@benesch
Copy link
Member

benesch commented Jun 18, 2021

I'm actually surprised we're not getting -0 in normal builds: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=3bfd6795728c2cf0d6f2108a5af91e37

@benesch
Copy link
Member

benesch commented Jun 18, 2021

I wonder if it's actually that a newer version of Rust (i.e., nightly) has changed how -0 is displayed.

@benesch
Copy link
Member

benesch commented Jun 18, 2021

Oh, yeah, here ya go: rust-lang/rust#78618. They changed it in Rust 1.53.

benesch added a commit to benesch/materialize that referenced this pull request Jun 18, 2021
In SQLite's SLT, floats are printed as integers by casting the float to
an integer then printing the integer. Our SLT runner was printing floats
rounding the float then printing the float with zero digits after the
decimal point. This is almost the same thing, except that "0" is printed
as "-0" since rust-lang/rust#78618. Future proof the code (and make it
work the same way in the nightly coverage build) by following the SQLite
approach.

Supersedes MaterializeInc#7096.
@benesch
Copy link
Member

benesch commented Jun 18, 2021

I sent #7102 to fix this.

benesch added a commit to benesch/materialize that referenced this pull request Jun 18, 2021
In SQLite's SLT, floats are printed as integers by casting the float to
an integer then printing the integer. Our SLT runner was printing floats
rounding the float then printing the float with zero digits after the
decimal point. This is almost the same thing, except that "0" is printed
as "-0" since rust-lang/rust#78618. Future proof the code (and make it
work the same way in the nightly coverage build) by following the SQLite
approach.

Supersedes MaterializeInc#7096.
@philip-stoev
Copy link
Contributor Author

Thank you both, I am abandoning this PR.

ruchirK pushed a commit to ruchirK/materialize that referenced this pull request Jun 21, 2021
In SQLite's SLT, floats are printed as integers by casting the float to
an integer then printing the integer. Our SLT runner was printing floats
rounding the float then printing the float with zero digits after the
decimal point. This is almost the same thing, except that "0" is printed
as "-0" since rust-lang/rust#78618. Future proof the code (and make it
work the same way in the nightly coverage build) by following the SQLite
approach.

Supersedes MaterializeInc#7096.
aljoscha pushed a commit to aljoscha/materialize that referenced this pull request Jul 21, 2021
In SQLite's SLT, floats are printed as integers by casting the float to
an integer then printing the integer. Our SLT runner was printing floats
rounding the float then printing the float with zero digits after the
decimal point. This is almost the same thing, except that "0" is printed
as "-0" since rust-lang/rust#78618. Future proof the code (and make it
work the same way in the nightly coverage build) by following the SQLite
approach.

Supersedes MaterializeInc#7096.
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.

None yet

3 participants