Jefffrey commented on code in PR #6912:
URL: https://github.com/apache/arrow-rs/pull/6912#discussion_r1902352166
##########
arrow-ord/src/rank.rs:
##########
@@ -177,6 +226,51 @@ mod tests {
assert_eq!(res, &[4, 6, 3, 6, 3, 3]);
}
+ #[test]
+ fn test_get_boolean_rank_index() {
+ assert_eq!(get_boolean_rank_index(true, true), 2);
+ assert_eq!(get_boolean_rank_index(false, true), 2);
+ assert_eq!(get_boolean_rank_index(true, false), 1);
+ assert_eq!(get_boolean_rank_index(false, false), 0);
+ }
+
+ #[test]
+ fn test_booleans() {
+ let descending = SortOptions {
+ descending: true,
+ nulls_first: true,
+ };
+
+ let nulls_last = SortOptions {
+ descending: false,
+ nulls_first: false,
+ };
+
+ let nulls_last_descending = SortOptions {
+ descending: true,
+ nulls_first: false,
+ };
+
+ let a = BooleanArray::from(vec![Some(true), Some(true), None,
Some(false), Some(false)]);
+ let res = rank(&a, None).unwrap();
+ assert_eq!(res, &[2, 2, 0, 1, 1]);
+
+ let res = rank(&a, Some(descending)).unwrap();
+ assert_eq!(res, &[1, 1, 0, 2, 2]);
+
+ let res = rank(&a, Some(nulls_last)).unwrap();
+ assert_eq!(res, &[1, 1, 2, 0, 0]);
+
+ let res = rank(&a, Some(nulls_last_descending)).unwrap();
+ assert_eq!(res, &[0, 0, 2, 1, 1]);
Review Comment:
This seems to be a dense_rank instead of rank which takes into account ties.
See the doctest example, where you can imagine `"foo"` as `true` and `"bar"`
as `false` (or vice-versa):
https://github.com/apache/arrow-rs/blob/b77d38d022079b106449ead3996f373edc906327/arrow-ord/src/rank.rs#L42-L46
I think the logic might need to be revisited for the boolean rank function?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
