Bypass naive ordering when ordering by key (natural leveldb order) #22
Conversation
ghost
assigned 
datastore.go
Outdated
| // leveldb iterators return entries sorted lexicographically by key, so we can bypass the naive ordering | ||
| // for OrderByKey. Since OrderByKey.Sort() has as struct receiver, the value stored in q.Orders can be a | ||
| // pointer or a struct, so we need to check for both for correctness. | ||
| if _, ok := o.(dsq.OrderByKey); ok { |
There was a problem hiding this comment.
Nit:
switch o.(type) {
case dsq.OrderByKey, *dsq.OrderByKey:
continue
}There was a problem hiding this comment.
Changed here and in ipfs/go-ds-badger#43 ;-)
| switch o.(type) { | ||
| case dsq.OrderByKey, *dsq.OrderByKey: | ||
| continue | ||
| } | ||
| qr = dsq.NaiveOrder(qr, o) |
There was a problem hiding this comment.
I believe we need OrderByKey to override other orders (this currently just skips these). Given that keys are unique and have a total order, I believe we should:
- Start at the end.
- Work backwards until we find the last
OrderByKey. - Apply any remaining orders.
There was a problem hiding this comment.
We could, in theory, optimize OrderByKeyDescending as well by taking advantage of the same total ordering. That is, stop at the last OrderByKeyDescending (if we reach one before we reach an OrderByKey). Unlike OrderByKey, we'll have to actually apply the OrderByKeyDescending order.
|
@Stebalien – I'll take a look, basically to honour the interface of However, I honestly fail to find a use case where someone would want to consume an iterator fully to then sort it in memory, before returning results to the caller, unless it's for very small slices, and even then you're probably better off handling this in client code and modeling the data to avoid such needs. Facilitating this in the public API with no warnings is a formula for horrendous performance served in a silver platter, if you ask me. |
|
BTW – are you aware of any consumers that use the ordering feature at all? An off-the-cuff search on go-ipfs returned nothing. |
|
Also, Order is a public interface which means that clients could supply custom Orders to queries, hence we have to think beyond OrderByKey* and OrderByValue* if we want to fully support this. |
|
@Stebalien – I get that OrderByKey* should be a terminating sort criteria. However, as I look at the ordering code and the combinatorics, the more I think it doesn't work as intended. My assumption is that multiple sorting criteria should work like in SQL, first order by X, then order by Y (which only takes effect on records with equal X). Unless I'm interpreting wrong, a sequence of ordering criteria, applied via naive order (behaviour as of today) will end up resorting the results, instead of applying a nested sorting. As a user, I would expect the following behaviour:
(same heuristic applies for Descending variants) I'm omitting other combinations, like repeating operators and such. The current code allows that. However, what I think happens currently is:
Am I reading it wrong? P.S.: apologies for rambling. |
|
Now I see what you mean. Yeah, that's what I'd also expect but that's definitely not what this does. Can you think of an interface you'd be happy with? At this point, we can probably make breaking changes as the interface is fundamentally broken. |
|
Superseded by #23 following interface change. |
Equivalent of ipfs/go-ds-badger#43 for leveldb, but simplified, as leveldb iterators don't allow specifying reverse order.
Needed for correctness in libp2p/go-libp2p-peerstore#47