ARROW-10864: [Rust] Use standard ordering for floats #8882

Dandandan · 2020-12-09T18:38:38Z

This implements ordering using IEEE 754 ordering as mentioned in this discussion (only for sort now).

I think this simplifies NaN-handling quite a bit. Performance-wise doesn't seem to be a big difference.

#8685

github-actions · 2020-12-09T18:57:21Z

https://issues.apache.org/jira/browse/ARROW-10864

codecov-io · 2020-12-09T18:58:27Z

Codecov Report

Merging #8882 (eac99bd) into master (dfef236) will decrease coverage by 0.00%.
The diff coverage is 68.62%.

@@            Coverage Diff             @@
##           master    #8882      +/-   ##
==========================================
- Coverage   82.60%   82.60%   -0.01%     
==========================================
  Files         204      204              
  Lines       50189    50177      -12     
==========================================
- Hits        41459    41448      -11     
+ Misses       8730     8729       -1

Impacted Files	Coverage Δ
rust/arrow/src/compute/kernels/sort.rs	`93.56% <68.62%> (+0.14%)`	⬆️
rust/parquet/src/encodings/encoding.rs	`95.24% <0.00%> (-0.20%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfef236...eac99bd. Read the comment docs.

nevi-me · 2021-01-02T07:33:29Z

@Dandandan what more work do you still need to do on this PR?

Dandandan · 2021-01-02T10:03:58Z

Thanks @nevi-me for remembering me. I updated and finalized the PR.

nevi-me

LGTM, needs rebase though :)

I've gone through the totalOrder logic, looks the same as what I've seen on the net.
I also saw this old RFC which would be useful by implementing Ord on floats, potentially giving us totalOrder in the standard library rust-lang/rfcs#1249.

Dandandan · 2021-01-03T11:29:37Z

@nevi-me yes, that is the info I also found.

There are also nightly implementations of total_cmp, we could switch whenever it is stabilized.
https://doc.rust-lang.org/std/primitive.f64.html#method.total_cmp

Dandandan · 2021-01-03T13:00:40Z

@nevi-me rebased, but looks like needs-rebase label isn't removed

jhorstmann · 2021-01-03T17:12:18Z

@Dandandan there is another implementation of float comparison in use for lexicographical ordering (sorting by multiple columns) in ord.rs. That would also need to be updated to be consistent.

Ideally the min/max kernels would also follow the same ordering. For the simd implementations that could be tricky because they currently delegate all types to the packed_simd lt implementation. I didn't have time to look into it in more detail yet.

In my understanding, the only difference in behaviour should be around negative NaN and that small difference shouldn't block this PR.

alamb · 2021-01-05T13:37:00Z

I will try and look at this PR later today

alamb

I read the code and verified we had test coverage for sorting of floating point nulls:

https://github.com/apache/arrow/blob/master/rust/arrow/src/compute/kernels/sort.rs#L1177

I vote Nice work @Dandandan

alamb · 2021-01-05T21:22:48Z

rust/arrow/src/compute/kernels/sort.rs

-    } else {
-        unreachable!("Partition by nan is only applicable to float types")
-    }
+// sorts f32 in IEEE 754 total ordering


It might be good here to provide a pointer to the original source implementation (e.g. https://doc.rust-lang.org/std/primitive.f64.html#method.total_cmp) and a TODO to change to use that API when is stabilized

I added comments here: #9129

…algorithm came from Follow up from #8882 Closes #9129 from alamb/alamb/doc-improvement Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Neville Dipale <nevilledips@gmail.com>

…algorithm came from Follow up from apache/arrow#8882 Closes #9129 from alamb/alamb/doc-improvement Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Neville Dipale <nevilledips@gmail.com>

This implements ordering using IEEE 754 ordering as mentioned in this discussion (only for sort now). I think this simplifies NaN-handling quite a bit. Performance-wise doesn't seem to be a big difference. apache#8685 Closes apache#8882 from Dandandan/float_order Authored-by: Heres, Daniel <danielheres@gmail.com> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

…algorithm came from Follow up from apache#8882 Closes apache#9129 from alamb/alamb/doc-improvement Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Neville Dipale <nevilledips@gmail.com>

github-actions bot added the Component: Rust label Dec 9, 2020

Dandandan marked this pull request as draft December 9, 2020 18:42

github-actions bot added the needs-rebase A PR that needs to be rebased by the author label Dec 11, 2020

Dandandan changed the title ~~ARROW-10864: [Rust] Use standard ordering for floats [WIP]~~ ARROW-10864: [Rust] Use standard ordering for floats Jan 2, 2021

Dandandan marked this pull request as ready for review January 2, 2021 10:06

nevi-me approved these changes Jan 3, 2021

View reviewed changes

nevi-me requested review from jorgecarleitao and andygrove January 3, 2021 06:22

Dandandan force-pushed the float_order branch from bfb14d7 to 750c911 Compare January 3, 2021 11:46

Dandandan added 7 commits January 3, 2021 13:28

Use standard ordering for floats

fa510cb

Renaming

6ebdd76

Add comments

dc03222

Remove pub

e898b22

lippy, simplify

079e3ff

Fix

c066d06

Fix rebase

eac99bd

Dandandan force-pushed the float_order branch from cb45867 to eac99bd Compare January 3, 2021 12:29

nevi-me removed the needs-rebase A PR that needs to be rebased by the author label Jan 3, 2021

nevi-me requested a review from alamb January 4, 2021 21:44

alamb approved these changes Jan 5, 2021

View reviewed changes

alamb closed this in 3ed90f6 Jan 7, 2021

alamb mentioned this pull request Jan 7, 2021

ARROW-11169: [Rust] Add a comment explaining where float total_order algorithm came from #9129

Closed

This was referenced Jan 7, 2021

[Rust] Use standard ordering for floats #26799

Closed

[Rust] Add a comment explaining where float total_order algorithm came from #27076

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-10864: [Rust] Use standard ordering for floats #8882

ARROW-10864: [Rust] Use standard ordering for floats #8882

Dandandan commented Dec 9, 2020 •

edited

github-actions bot commented Dec 9, 2020

codecov-io commented Dec 9, 2020 •

edited

nevi-me commented Jan 2, 2021

Dandandan commented Jan 2, 2021

nevi-me left a comment

Dandandan commented Jan 3, 2021

Dandandan commented Jan 3, 2021

jhorstmann commented Jan 3, 2021

alamb commented Jan 5, 2021

alamb left a comment

alamb Jan 5, 2021

alamb Jan 7, 2021

ARROW-10864: [Rust] Use standard ordering for floats #8882

ARROW-10864: [Rust] Use standard ordering for floats #8882

Conversation

Dandandan commented Dec 9, 2020 • edited

github-actions bot commented Dec 9, 2020

codecov-io commented Dec 9, 2020 • edited

Codecov Report

nevi-me commented Jan 2, 2021

Dandandan commented Jan 2, 2021

nevi-me left a comment

Choose a reason for hiding this comment

Dandandan commented Jan 3, 2021

Dandandan commented Jan 3, 2021

jhorstmann commented Jan 3, 2021

alamb commented Jan 5, 2021

alamb left a comment

Choose a reason for hiding this comment

alamb Jan 5, 2021

Choose a reason for hiding this comment

alamb Jan 7, 2021

Choose a reason for hiding this comment

Dandandan commented Dec 9, 2020 •

edited

codecov-io commented Dec 9, 2020 •

edited