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

NaN handling broke in 1.0.6 #153

Closed
astraw opened this issue Apr 29, 2019 · 4 comments
Closed

NaN handling broke in 1.0.6 #153

astraw opened this issue Apr 29, 2019 · 4 comments

Comments

@astraw
Copy link

astraw commented Apr 29, 2019

What version of the csv crate are you using?

The problem arose in 6105a6a, which was released as 1.0.6.

Briefly describe the question, bug or feature request.

Not a number values of f64 ("NaN") are no longer serialized as "NaN" but rather an extreme value such as "2.696539702293474e308". I have not checked if this also affects f32.

Include a complete program demonstrating a problem.

Change the tutorial tutorial-write-serde-02.rs like this:

--- examples/tutorial-write-serde-02.rs	2019-04-29 01:50:05.089292433 +0200
+++ ../csv-nan-demo/src/main.rs	2019-04-29 02:03:24.364042987 +0200
@@ -1,3 +1,4 @@
+//tutorial-write-serde-02.rs
extern crate csv;
extern crate serde;
#[macro_use]
@@ -39,7 +40,7 @@
        city: "Oakman",
        state: "AL",
        population: None,
-        latitude: 33.7133333,
+        latitude: std::f64::NAN,
        longitude: -87.3886111,
    })?;

What is the observed behavior of the code above?

I now get the following output:

City,State,Population,Latitude,Longitude
Davidsons Landing,AK,,65.2419444,-165.2716667
Kenai,AK,7610,60.5544444,-151.2583333
Oakman,AL,,2.696539702293474e308,-87.3886111

Note the extreme latitude value for Oakman.

What is the expected or desired behavior of the code above?

whereas with the previous commit, I got:

City,State,Population,Latitude,Longitude
Davidsons Landing,AK,,65.2419444,-165.2716667
Kenai,AK,7610,60.5544444,-151.2583333
Oakman,AL,,NaN,-87.3886111
@astraw
Copy link
Author

astraw commented May 30, 2019

The documentation for ryu::Buffer::format says:

/// # Special cases
///
/// This function **does not** check for NaN or infinity. If the input
/// number is not a finite float, the printed representation will be some
/// correctly formatted but unspecified numerical value.
///
/// Please check [`is_finite`] yourself before calling this function, or
/// check [`is_nan`] and [`is_infinite`] and handle those cases yourself.
///
/// [`is_finite`]: https://doc.rust-lang.org/std/primitive.f64.html#method.is_finite
/// [`is_nan`]: https://doc.rust-lang.org/std/primitive.f64.html#method.is_nan
/// [`is_infinite`]: https://doc.rust-lang.org/std/primitive.f64.html#method.is_infinite

it looks like serialize_f32() and serialize_f64() for SeRecord in src/serializer.rs use this method but do not make this check. I think either the check should be made or lower-level functions, which do format NaN correctly, should be used. These lower level functions are f2s_buffered_n and d2s_buffered_n.

@BurntSushi
Copy link
Owner

I see. Yeah, I think doing the explicit check with is_finite/is_nan would be the best path forward here. The lower level functions look handy, but I'd much rather do the extra case analysis and avoid unsafe if we can get away with it.

I also filed an issue upstream to see about perhaps fixing this inside of ryu (or at least get a better understanding of the current design): dtolnay/ryu#12

@jesskfullwood
Copy link

I believe this issue can now be resolved simply by bumping ryu to 1.0.0.

@BurntSushi
Copy link
Owner

@jesskfullwood Awesome, thanks for the tip. I'll do the upgrade when I context switch back into this library, since there are a few other things I'd like to tidy up, and then do a 1.1.0 release. It's next on my list.

BurntSushi added a commit that referenced this issue Jun 26, 2019
We fix NaN handling by upgrading ryu to 1.0, which by default formats
NaN/non-finites for us.

Fixes #153
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

No branches or pull requests

3 participants