P-54 Binary search for holding time assertion

[grumpygreenguy]

Context

Reduce the number of data provider requests needed to implement the "holding time" assertion.

Resolves P-54

How (Optional)

The basic idea is to do a binary search over the range of relevant dates, testing every relevant address on each iteration, and discarding any addresses that prove to be irrelevant, that is, where we know for sure that the longest holding time is not held by that address.

An address is proven irrelevant if:

• The data provider query returns false for that address on a given date
• The data provider query returns true for another address on the same date

Note on Error Handling

Since the goal is to find the earliest holding date (and not necessarily to identify the corresponding address), it is OK to ignore certain errors as long as the search can proceed. In a nutshell, the search can only get "stuck" (i.e. we don't know whether to choose the earlier or the later half of the date range) if:

• No data provider query returns true
• At least one query returned an error.

In that case, the actual value of the failed query may have been true or false, so we don't know in which direction the search should continue.

If at least one query returned true, we know that the search continues in the earlier half; at worst we are failing to remove some irrelevant addresses that will be discarded in the subsequent step when the query returns false for them.

TODO

• Pass CI

[linear]

P-54 Reduce number of requests in HOLDER assertion building

[Kailai-Wang]

Thank you, the intention is quite clear!

Have you considered partition_point? It applies the binary search under the hood too.

We can treat the date array as "sorted"/ partitioned because holding_time(index) == true implies holding_time(index + 1) == true (but not vice versa). If that's not the case in practice, then data provider malfunctions and you won't get the right result anyway.

[Kailai-Wang]

Can we merge it into the accounts.iter() traverse above? I feel it should be possible

[grumpygreenguy]

Possibly, but I'm not sure it's worth it. We only do the filtering if any of the queries returned true in that iteration; if we want to merge the two loops we'd have to speculatively build the filtered array in advance, and I think that would make the logic messier to follow. In terms of runtime, I don't think the added loop to construct the filtered list makes much of a difference; what dominates here is in any case the query provider requests.

[grumpygreenguy]

@Kailai-Wang

Thank you, the intention is quite clear!

Thanks!

Have you considered partition_point? It applies the binary search under the hood too.

Didn't know about it; will have a look! Thanks

We can treat the date array as "sorted"/ partitioned because holding_time(index) == true implies holding_time(index + 1) == true (but not vice versa). If that's not the case in practice, then data provider malfunctions and you won't get the right result anyway.

Exactly; that's the entire justification for the search algorithm.

[Kailai-Wang]

partition_point is basically a binary search by predicate, applying it would make the code simpler (so better readability) and less error-prone (as it's a library func) IMO

[grumpygreenguy]

partition_point is basically a binary search by predicate, applying it would make the code simpler (so better readability) and less error-prone (as it's a library func) IMO

On further look, there's a problem with error handling -- the predicate arg to partition_point must return bool, but the predicate depends on data provider queries which could fail, so the search logic needs to be able to handle that (at the very least, to bubble the error without panicking)

[Kailai-Wang]

We should be able to use additional flag/storage to tackle that 🤔

[grumpygreenguy]

We should be able to use additional flag/storage to tackle that 🤔

Care to elaborate on that? Or, do you have a link with some info? Because from the API docs (and some cursory search) I haven't found any way to work around that (without reimplementing the search algorithm) 🤔

[Kailai-Wang]

partition_point might produce more readable code, but let's leave it to another PR even if that's the case

[outofxxx]

Hi, @grumpygreenguy, It's good to have this try, thanks. and i have a question, how much can requests be reduced in the worst-case scenario after this optimization?

[grumpygreenguy]

@zhouhuitian

Hi, @grumpygreenguy, It's good to have this try, thanks. and i have a question, how much can requests be reduced in the worst-case scenario after this optimization?

The complexity goes from linear in the number of dates to logarithmic in the same; specifically, if we're checking for a single address, the number of requests in the worst case goes from 15 to 5.
If there are multiple addresses to check, the complexity is still in the worst case linear in the number of addresses as well, but depending on how quickly the search can spot and eliminate irrelevant addresses, the cost of each iteration can be drastically reduced. Would need to have a better idea of the distribution of holding times between multiple accounts of real users to make an estimate of the average runtime, tho.

[grumpygreenguy]

Thanks @Kailai-Wang for fixing this issue! Still gotta wrap my head around some of the subtleties here, apparently :D

In this case the extra clone shouldn't make a huge difference overall, since the cost is in any case dominated by the HTTP requests; in general though it does feel a bit wasteful to have to clone the array each time, only to replace the original anyway after the call to holding_time_search_step. What would (in theory) have been the better approach here? (Not for this PR to be sure, just as an overall learning)