anshumg commented on PR #5108:
URL: https://github.com/apache/solr/pull/5108#issuecomment-6656821338
Thank you @nick-boss-tech .
This PR looks good specially,
- The forward-token compatibility is well thought out. Existing
nextCursorMark values keep their encoding, so older nodes and clients aren't
affected.
- The distributed handling is well done. Flipping positionInResponse after
the merge is clean, and the tests that force documents onto competing shards
(including the tie case) really neat.
- SortSpecReverseTest has pretty good coverage: real collectors,
multi-valued selectors, missing values, and searchAfter in reverse.
I want to raise one design question before we go further: where the sort
reversal itself should live. Producing the exact inverse of a Lucene Sort is a
Lucene-level capability, and that was the motivation behind the long running
Lucene PR apache/lucene#14775, intending to use Sort.inverse().
That PR stalled, which I think is why ReverseSortField ended up in Solr.
Over the years Solr has collected a number of workarounds for things that
belonged in Lucene, and I'd like to avoid adding another.
Additionally for backwards cursors, page N going backwards must contain
exactly the documents of page N going forwards. That only works if two things
stay the same for every document and only the comparison direction flips:
1. Which value a multi-valued doc is sorted by. A doc with tags=[1, 9] is
sorted by either 1 (the minimum) or 9 (the maximum).
2. Where docs with no value go.
Solr re-parsing the sort changes both.
When Solr builds a sort from sort=tags asc or sort=tags desc, it picks both
of those based on the direction:
- Which value: ascending uses the minimum, descending uses the maximum
(PrimitiveFieldType:49, SortableTextField:187).
- Missing values: with sortMissingLast, docs with no value go last in both
directions (FieldType.missingValue(), line 856).
So you can't reverse a sort by just re-parsing it with the opposite
direction. Example with doc A tags=[1,9], doc B tags=[5], doc C with no tags,
and sortMissingLast:
Sort Value used per doc
Order
tags asc A=1, B=5
A, B, C
tags desc, re-parsed by Solr A=9, B=5 A, B, C
exact reverse of asc A=1, B=5 C,
B, A
P.S: I hope the example renders fine as it seems ok when I'm typing it.
The re-parsed desc comes back in the same order as asc, so it's nothing like
the reverse.
By the time Lucene runs the query, tags asc has become a fully built Lucene
sort field: sort by the minimum value, ascending, with an explicit "missing"
setting. #14775 builds a copy with only the direction flipped and everything
else unchanged. Every doc keeps its sort value (A=1, B=5), and flipping the
direction also moves the missing docs to the opposite end. The result is C, B,
A, which is exactly right. So the Lucene approach doesn't hit the problem the
comment describes.
From what I see, your work carries over directly to Lucene. Two pieces of
this PR would make the Lucene change better, and I'd love for you to contribute
them there yourself if you're interested:
1. ReverseSortField as the default SortField.inverseSort(). Right now the
default in #14775 rebuilds a plain SortField, which silently loses any custom
comparator in a subclass that doesn't override it. Your delegating wrapper is
always correct, so it's the right fallback. The specific overrides would keep
skipping for the common types.
2. SortSpecReverseTest as a Lucene test. It only uses Lucene APIs, so it
would port almost unchanged, and in my opinion, it's better than the tests
currently in #14775.
Hope this makes sense.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
