Significant loss of precision with `duration` literals using a float

NotTheDr01ds commented 1 year ago

Describe the bug

This might just be "as designed", but there's a much more significant loss of precision in duration than I would expect when specifying a decimal value for a duration-literal.

(Sorry, on a bit of a duration kick today apparently) ;-)

How to reproduce

> 1.5wk
1wk 3day
> 1.5wk == 10.5day
false
# However, the following works as expected
> 1wk * 1.5
1wk 3day 12hr
> 2.0wk == 2wk
true
> 2wk == (1wk * 2.0)
true
# But
> 1.5wk == (1wk * 1.5)
false

Expected behavior

> 1.5wk == (1wk * 1.5)
true
> 1.5wk
1wk 3day 12hr

Screenshots

No response

Configuration

key	value
version	0.83.1
branch
commit_hash
build_os	linux-x86_64
build_target	x86_64-unknown-linux-gnu
rust_version	rustc 1.71.0 (8ede3aae2 2023-07-12)
rust_channel	stable-x86_64-unknown-linux-gnu
cargo_version	cargo 1.71.0 (cfd3bbd8f 2023-06-08)
build_time	2023-07-31 18:55:46 -04:00
build_rust_channel	release
allocator	standard
features	dataframe, default, extra, sqlite, trash, which, zip
installed_plugins

Additional context

Perhaps not a big priority for now - Obvious workaround is to simply use integer units for the literal and then multiply/divide as needed. However, there might be some corner-cases where this won't be an option.

fdncred commented 1 year ago

@NotTheDr01ds Seems like you'd be a good tester with some feedback on this PR since it's in the same vein https://github.com/nushell/nushell/pull/9632

NotTheDr01ds commented 1 year ago

@fdncred Oh - Thanks! I had skimmed that PR a few days ago but it hadn't "stuck" when I came across this. Definitely in the same vein, and I'll take a closer look.

bobhy commented 1 year ago

Simpler repro... it looks like the expression 0.5wk produces an incorrect value. Repros on main, doesn't have anything to do with #9632 (yet).

> 1wk | into int
604800000000000
> 7 * 24 * 3600 * 1_000_000_000 # 1 week's worth of nanoseconds
604800000000000
> (1wk | into int ) * 1.5
907200000000000
> (1.0wk + 0.5wk) | into int # 1.5wk is not the same as above.  Which is right?
864000000000000 
> 7 * 24 * 3600 * 1_000_000_000 / 2 # half week's worth of nanoseconds, so 907200000 was right.
302400000000000
> 0.5wk | into int  # this produces too-small result.
259200000000000

So we need to dig into why 0.5wk produces too-small result.

bobhy commented 1 year ago

This behavior is as designed, in the parser. See: https://github.com/nushell/nushell/blob/f61503893835f777c1ade71ec76a6b07fcf818f2/crates/nu-parser/src/parser.rs#L2270C9-L2276C11 When you provide fractional units, it converts to an integer number of the next smaller time unit with truncation. So...

> 0.5wk
3day

was (0.5 * 7 days / week == 3.5 days) truncate to int == 3day == 259200000000000ns

The advantage of this behavior is that it displays a duration with the next smaller time unit, which is more user-friendly. The disadvantage, as you see, is that the fractional unit values are not really reliable. This behavior is worst for inputs in weeks, because there are only 7 days, so the effect of integer truncation is most noticeable. Here are some worst-case examples:

> 0.5wk
3day
> 0.25wk
1day
> 0.125wk
0sec
> 0.1.5wk
1day

We could fix this by always converting a fractional unit to the smallest time unit, e.g 0.5wk == 302400000000000ns. Simple, but ugly. Or by converting to a fractional number of the next smaller unit, e.g, 0.5wk == 3.5day.
But I think the intent of the code is to show an integer number of some time unit, so maybe 0.5wk == 84hr.

What do you think makes the most sense?

nushell / nushell