Add ArrayUtils.binarySearch() with a key extractor #1270

kvr000 · 2024-08-31T19:54:42Z

There is Collections.binarySearch() in Java core. However, it searches for exact element which is often not usable, as the elements are typically sorted by particular field.

These methods introduce binarySearch() on arrays and (sorted) List, with ability to provide sorting key extraction function and key comparator.

The rest of semantics, behavior, parameters and return values remain consistent with Collections.binarySearch() from Java core.

garydgregory

Hello @kvr000
Thank you for your PR.
Please see my comments.
Waiting for community feedback as well...

src/main/java/org/apache/commons/lang3/SortedArrayUtils.java

garydgregory · 2024-09-03T12:36:51Z

src/main/java/org/apache/commons/lang3/SortedListUtils.java

+ *
+ * @author Zbynek Vyskovsky
+ */
+public class SortedListUtils {


Collection utilities do not belong in Commons Lang IMO. This would be for Commons Collections unless this functionality is already there.

I removed it. Unsure whether this was the best choice, as this is about the algorithm, not about implementing collections.

src/main/java/org/apache/commons/lang3/SortedListUtils.java

garydgregory

Hello @kvr000,

This code should be in ArrayUtils.
TY!

kvr000 · 2024-09-11T04:43:41Z

Hello @kvr000,

This code should be in ArrayUtils. TY!

@garydgregory : Yeah, got it, was busy to finish that. Fixed now, thank you.

garydgregory · 2024-09-11T13:40:02Z

Hello @kvr000,
This code should be in ArrayUtils. TY!

@garydgregory : Yeah, got it, was busy to finish that. Fixed now, thank you.

@kvr000

Run mvn (by itself) before you push to catch build errors.

garydgregory

Hello @kvr000

See my scattered comments.
Add a method called createSortedList(int...) that guarantees sorted results and use it.
Add tests for non-sorted input, make sure the results are predictable, we do not want a DOS for an infinite loop for example.

src/test/java/org/apache/commons/lang3/ArrayUtilsBinarySearchTest.java

src/main/java/org/apache/commons/lang3/ArrayUtils.java

garydgregory · 2024-09-11T20:10:23Z

src/test/java/org/apache/commons/lang3/ArrayUtilsBinarySearchTest.java

+public class ArrayUtilsBinarySearchTest extends AbstractLangTest {
+
+    @Test
+    public void binarySearch_whenEmpty_returnM1() {


Since all tests in the class call binarySearch() and the class is already called "binary search", you do not need to repeat that name in each test method name, that'll de-clutter names a bit.

I don't agree with this comment. The methods names are intentionally descriptive with pattern name-condition-expected. This improves readability. While there are truly only binarySearch now, it doesn't have to be forever and vice versa - this class may be possibly merged into more generic ones which would work with sorted arrays (which was the very original name).

kvr000 · 2024-09-12T02:39:06Z

Hello @kvr000

See my scattered comments.

Add a method called createSortedList(int...) that guarantees sorted results and use it.

Add tests for non-sorted input, make sure the results are predictable, we do not want a DOS for an infinite loop for example.

@garydgregory :

The createdSortedList() won't have any value. In the test, I have to see what is returned index - that's impossible with unsorted array. Additionally, now it's used also for intentionally unsorted array.

Good point for non-sorted input, I added a test. The code always does l = m+1 or h = m-1 , so it's safe.

sebbASF

Some documentation clarifications

sebbASF · 2024-09-12T07:32:58Z

src/main/java/org/apache/commons/lang3/ArrayUtils.java

+     * @return
+     *      index of the search key, if it is contained in the array within specified range; otherwise,
+     *      (-first_greater - 1).  The first_greater is the index of lowest greater element in the list - if all elements
+     *      are lower, the first_greater is defined as toIndex.


The above para needs rework: there is no specified range here, and no toIndex

The result should be documented as undefined if all array elements are equals; like the Arrays version.

Updated to match non-index version.

@garydgregory : That is great point. In fact, one of my use cases is array (list) with duplicities. So this can be done better. I introduce binarySearchFirst and binarySearchLast methods which return consistently first and last matching element respectively.

WRT binarySearchFirst and Last, let's not add those.

@garydgregory : I already did. I dropped binarySearch as the randomness of choosing any of equal element effectively disqualifies it from such use cases. Predictable behavior of binarySearchFirst and binarySearchLast is huge benefit and comes at almost no cost (is slower only in cases when the lookup is quickly "lucky").

The use caee, I thought, was a version of the 'Arrays' method plus key extraction. I would not necessarily want an API that always allocates an array to return a value. Maybe do this later, as a second API only if needed.

In the case of the 'Arrays' method, the entire entry is matched, so to some extent it does not matter which index is returned. That is not the case where a partial key is used, so I think in the user will often want to know all the matches.

Sudden thought: surely key extraction can take place in the Comparator, so why is this method needed at all?

Or maybe I am missing something?

Sudden thought: surely key extraction can take place in the Comparator, so why is this method needed at all?

Or maybe I am missing something?

Good point @sebbASF !

We might not need this at all...

@sebbASF @garydgregory :

In the case of the 'Arrays' method, the entire entry is matched, so to some extent it does not matter which index is returned. That is not the case where a partial key is used, so I think in the user will often want to know all the matches.

In my particular case, I have list of GPS locations sorted by time and track distance, with distance unchanged if there was no move (causing duplicates). One use case is to find point for particular distance and if there are duplicates, I want the result to be predictable (here last but can be first in different use cases). If the result is unpredictable, it will lead to different results based on initial conditions for the search. I don't care about whole range, though I can imagine someone would appreciate such function with added complexity.

Sudden thought: surely key extraction can take place in the Comparator, so why is this method needed at all?

No, unless we're able to construct the full object which is not always possible - note that Comparator takes two arguments of the same type. It is technically achievable with hacking and casting one argument to T and the other one to K, relying on internal implementation passing the arguments in expected order but that would be super ugly and fragile to implementation changes.

@garydgregory :

Please revert the First/Last APIs.

For the reasons above, I want to keep those as they better match intended use case. I think we actually don't need original unpredictable binarySearch() .

src/main/java/org/apache/commons/lang3/ArrayUtils.java

sebbASF · 2024-09-12T07:44:33Z

src/main/java/org/apache/commons/lang3/ArrayUtils.java

+                return m;
+            }
+        }
+


What if there are no higher matches?

Then l reaches h+1 and exits the loop, returning -toIndex - 1 as documented.

…licit elements

garydgregory · 2024-09-13T14:37:54Z

@kvr000
Please revert the First/Last APIs.

kvr000 · 2024-09-18T03:28:39Z

@kvr000 Please revert the First/Last APIs.

@garydgregory : As explained in #1270 , I want to keep First/Last as I find them more useful due to their predictability. I don't think we really need original unpredictable simple binarySearch.

binarySearch on sorted arrays and sorted lists

146551b

garydgregory requested changes Sep 3, 2024

View reviewed changes

kvr000 force-pushed the sorted-list-binary-search branch 2 times, most recently from ed7f900 to ae97b6f Compare September 4, 2024 01:42

Remove SortedListUtils binarySearch

76e2a2c

garydgregory requested changes Sep 11, 2024

View reviewed changes

kvr000 force-pushed the sorted-list-binary-search branch from e974c36 to d3e3623 Compare September 11, 2024 04:42

garydgregory requested changes Sep 11, 2024

View reviewed changes

garydgregory reviewed Sep 11, 2024

View reviewed changes

kvr000 force-pushed the sorted-list-binary-search branch 4 times, most recently from 4c16d1a to 9063592 Compare September 12, 2024 02:32

sebbASF requested changes Sep 12, 2024

View reviewed changes

garydgregory changed the title ~~binarySearch on sorted arrays and sorted lists~~ binarySearch on sorted arrays Sep 12, 2024

Add binarySearchFirst and binarySearchLast for stable search with dup…

f56bf0a

…licit elements

kvr000 force-pushed the sorted-list-binary-search branch from 9063592 to f56bf0a Compare September 13, 2024 02:46

kvr000 mentioned this pull request Sep 13, 2024

Sorted List binarySearch functions apache/commons-collections#542

Open

garydgregory changed the title ~~binarySearch on sorted arrays~~ Add ArrayUtils.binarySearch with a key extractor Sep 13, 2024

garydgregory changed the title ~~Add ArrayUtils.binarySearch with a key extractor~~ Add ArrayUtils.binarySearch() with a key extractor Sep 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ArrayUtils.binarySearch() with a key extractor #1270

Add ArrayUtils.binarySearch() with a key extractor #1270

kvr000 commented Aug 31, 2024 •

edited

Loading

garydgregory left a comment

garydgregory Sep 3, 2024

kvr000 Sep 4, 2024

garydgregory left a comment

kvr000 commented Sep 11, 2024 •

edited

Loading

garydgregory commented Sep 11, 2024

garydgregory left a comment

garydgregory Sep 11, 2024

kvr000 Sep 12, 2024

kvr000 commented Sep 12, 2024 •

edited

Loading

sebbASF left a comment

sebbASF Sep 12, 2024

garydgregory Sep 12, 2024 •

edited

Loading

kvr000 Sep 13, 2024

garydgregory Sep 13, 2024

kvr000 Sep 13, 2024

garydgregory Sep 13, 2024

sebbASF Sep 13, 2024

sebbASF Sep 13, 2024

garydgregory Sep 13, 2024

kvr000 Sep 13, 2024 •

edited

Loading

sebbASF Sep 12, 2024

kvr000 Sep 13, 2024

garydgregory commented Sep 13, 2024

kvr000 commented Sep 18, 2024

Add ArrayUtils.binarySearch() with a key extractor #1270

Are you sure you want to change the base?

Add ArrayUtils.binarySearch() with a key extractor #1270

Conversation

kvr000 commented Aug 31, 2024 • edited Loading

garydgregory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garydgregory left a comment

Choose a reason for hiding this comment

kvr000 commented Sep 11, 2024 • edited Loading

garydgregory commented Sep 11, 2024

garydgregory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvr000 commented Sep 12, 2024 • edited Loading

sebbASF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garydgregory Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvr000 Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garydgregory commented Sep 13, 2024

kvr000 commented Sep 18, 2024

kvr000 commented Aug 31, 2024 •

edited

Loading

kvr000 commented Sep 11, 2024 •

edited

Loading

kvr000 commented Sep 12, 2024 •

edited

Loading

garydgregory Sep 12, 2024 •

edited

Loading

kvr000 Sep 13, 2024 •

edited

Loading