Support descending queries #3499
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt/bug
branch
from
661aab8 to
a260532
Compare
I am really struggling to understand why this was broken and not detected by the existing tests. I also wonder whether this is actually the right fix and could use a thorough set of eyes.
schmidt-sebastian
force-pushed
the
mrschmidt/bug
branch
from
a260532 to
e8da9bb
Compare
Coverage Report 1Affected Products
Test Logs
Notes |
Size Report 1Affected Products
Test Logs
Notes |
| .orderBy(orderBy("c", "desc")) | ||
| .orderBy(orderBy("b", "asc")) | ||
| .orderBy(orderBy("a", "desc")) | ||
| .limitToFirst(2); |
There was a problem hiding this comment.
I would remove limitToFirst as it's not the intent of the test.
For completion, I'd suggest testing the 4 combinations:
c > 2andc DESC(already covered here)c > 2andc ASCc < 5andc ASCc < 5andc DESC
There was a problem hiding this comment.
Ideally we should also add some test cases that have startAt/endAt for each of them
There was a problem hiding this comment.
The limitToLast() was there since verifyResults did not validate ordering, and this was a quick hack to ensure that the ordering here worked. I changed the IndexManager API to return an ordered list instead of a Set. This allowed me to remove the limit here and validate the ordering of the result.
| if (max(segmentValue, cursorValue) == cursorValue) { | ||
| segmentValue = cursorValue; | ||
| segmentInclusive = startAt.isInclusive(); | ||
| if (max(segmentBound.first, cursorValue) == cursorValue) { |
There was a problem hiding this comment.
took me a minute to realize why we always do max here, and not decide whether we should do min or max based on the segment kind.
We should add tests that contain startAt and endAt.
There was a problem hiding this comment.
I added tests, which actually showed that this logic is wrong (I compared the result with the Java Server SDK for validation). This is now part of the getAscendingBound() logic.
|
/run binary-size |
|
/test smoke-tests |
|
@schmidt-sebastian: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
I am really struggling to understand why this was broken and not detected by the existing tests. I also wonder whether this is actually the right fix and could use a thorough set of eyes.