Support descending queries #6075
Conversation
|
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (746,042 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
schmidt-sebastian
force-pushed
the
mrschmidt/descorder
branch
from
f30d19a to
c2ab3f5
Compare
| } | ||
| break; |
There was a problem hiding this comment.
We should port this change (adding a break statement) to the Android SDK.
| segmentValue = filterValue; | ||
| segmentInclusive = filterInclusive; | ||
| } | ||
| function targetGetAscendingBound( |
There was a problem hiding this comment.
Add comments about what the function does.
There was a problem hiding this comment.
same for targetGetDescendingBound
| }); | ||
| }); | ||
| } | ||
| ); | ||
| }).next(() => result as DocumentKeySet | null); | ||
| }).next(() => result as DocumentKey[] | null); |
There was a problem hiding this comment.
is as DocumentKey[] needed here? result is already of type DocumentKey[]
There was a problem hiding this comment.
We are casting to DocumentKey[] | null. This means that the function returns PersistencePromise<DocumentKey[] | null> rather than PersistencePromise<DocumentKey[]> | PersistencePromise<null>, which is unfortunately a differen type.
| @@ -79,6 +79,18 @@ export function fieldIndexGetDirectionalSegments( | |||
| return fieldIndex.fields.filter(s => s.kind !== IndexKind.CONTAINS); | |||
| } | |||
|
|
|||
| /** | |||
| * Returns the order of the document key component. | |||
There was a problem hiding this comment.
Returns the order of the document key component for the given index
| @@ -906,15 +935,17 @@ export class IndexedDbIndexManager implements IndexManager { | |||
| this.uid, | |||
| bounds[i].arrayValue, | |||
| bounds[i].directionalValue, | |||
| new Uint8Array(), | |||
There was a problem hiding this comment.
I'm guessing the encoding of an empty string is an empty byte array
There was a problem hiding this comment.
OrderedCodeWriter writes bytes, so this is the byte array that sorts before any other byte arrays.
| * The document key this entry points to. This entry is encoded by an ordered | ||
| * encoder to match the key order of the index. | ||
| */ | ||
| orderedDocumentKey: Uint8Array; |
There was a problem hiding this comment.
Is it true that if the order was always ASC, then using EncodedResourcePath below would have sufficed? in order words: is the only reason we need this is because of DESC indexes?
There was a problem hiding this comment.
Yes. EncodedResourcePath already does some gymnastics to make sure that paths order naturally, even if there the slashes would yield our of order results normally. With EncodedResourcePath, we get: a/b, a/c, aa/c instead of a/b, aa/b, a/c for example (example-ish, since the sorting is off here too)
There was a problem hiding this comment.
FWIW, I changed the documentKey entry to no longer use EncodedResourcePath since we no longer rely on any ordering.
Changeset File Check
|
|
I fixed the test failures. Sorry for not catching them earlier. I think we should write better test helpers to reduce the repetitiveness of the bounds tests, but I can't get myself to do that on my current setup. |
This adds code to order all query results by document key, which is needed to return the correct result for limit queries.
Port of firebase/firebase-android-sdk#3499