Make sqllogictest platform-independent for the sign of NaN #7462
Conversation
| query R | ||
| SELECT * FROM test_float WHERE column1 IN (0.0, 1.2, 'NaN'::double, '-NaN'::double) | ||
| query T | ||
| SELECT column1 FROM test_float WHERE column2 IN (0.0, 1.2, 'NaN'::double, '-NaN'::double) |
There was a problem hiding this comment.
Modified this test which is sign sensitive not to show NaN and -NaN.
| } else { | ||
| "-NaN".to_string() | ||
| } | ||
| "NaN".to_string() |
There was a problem hiding this comment.
Hmm, you mean on different platform, a same value could be positive and negative?
There was a problem hiding this comment.
If so, maybe it good to leave a comment here.
There was a problem hiding this comment.
Yes. a function/operation can return -NaN on x86 while NaN on Arm.
| # select NaNs | ||
| query RRRR | ||
| select 'NaN'::double a, '-NaN'::double b, 'NaN'::float c, '-NaN'::float d | ||
| ---- | ||
| NaN -NaN NaN -NaN |
There was a problem hiding this comment.
Even simply literals also are impacted, i.e. platform dependent?
There was a problem hiding this comment.
I don't think so.
I believe NaN and -NaN are finally parsed here.
So, as long as f32::NAN, f64::NAN, -f32::NAN and -f64::NAN follows IEEE 754, simple literals should not be impacted.
But the string representation needs to be NaN regardless of NaN or -NaN.
So this test should be changed.
There was a problem hiding this comment.
I think we should leave the test to show the behavior of parsing -NaN.
There was a problem hiding this comment.
|
Hi @sarutak. |
| # select NaNs | ||
| query RRRR | ||
| select 'NaN'::double a, '-NaN'::double b, 'NaN'::float c, '-NaN'::float d | ||
| ---- | ||
| NaN -NaN NaN -NaN |
There was a problem hiding this comment.
I think we should leave the test to show the behavior of parsing -NaN.
|
Looks good to me -- thank you @sarutak |
4 participants
Which issue does this PR close?
Closes #7458
Rationale for this change
The sign of
NaNreturned from a function seems to depend on platform.But it's better for
sqllogictestto be platform-independent.What changes are included in this PR?
Revert #7403 and change one test which is sign sensitive .
Are these changes tested?
cargo test --test sqllogictestspassed.Are there any user-facing changes?
No.