Skip to content

Fix an issue where TargetIndexMatcher rejects valid queries #7335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions packages/firestore/src/model/target_index_matcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,18 +118,19 @@ export class TargetIndexMatcher {
}

const segments = fieldIndexGetDirectionalSegments(index);
let equalitySegments = new Set<string>();
let segmentIndex = 0;
let orderBysIndex = 0;

// Process all equalities first. Equalities can appear out of order.
for (; segmentIndex < segments.length; ++segmentIndex) {
// We attempt to greedily match all segments to equality filters. If a
// filter matches an index segment, we can mark the segment as used.
// Since it is not possible to use the same field path in both an equality
// and inequality/oderBy clause, we do not have to consider the possibility
// that a matching equality segment should instead be used to map to an
// inequality filter or orderBy clause.
if (!this.hasMatchingEqualityFilter(segments[segmentIndex])) {
if (this.hasMatchingEqualityFilter(segments[segmentIndex])) {
equalitySegments = equalitySegments.add(
segments[segmentIndex].fieldPath.canonicalString()
);
} else {
// If we cannot find a matching filter, we need to verify whether the
// remaining segments map to the target's inequality and its orderBy
// clauses.
Expand All @@ -144,16 +145,22 @@ export class TargetIndexMatcher {
return true;
}

// If there is an inequality filter, the next segment must match both the
// filter and the first orderBy clause.
if (this.inequalityFilter !== undefined) {
const segment = segments[segmentIndex];
// If there is an inequality filter and the field was not in one of the
// equality filters above, the next segment must match both the filter
// and the first orderBy clause.
if (
!this.matchesFilter(this.inequalityFilter, segment) ||
!this.matchesOrderBy(this.orderBys[orderBysIndex++], segment)
!equalitySegments.has(this.inequalityFilter.field.canonicalString())
) {
return false;
const segment = segments[segmentIndex];
if (
!this.matchesFilter(this.inequalityFilter, segment) ||
!this.matchesOrderBy(this.orderBys[orderBysIndex++], segment)
) {
return false;
}
}

++segmentIndex;
}

Expand Down
63 changes: 63 additions & 0 deletions packages/firestore/test/unit/local/index_manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -939,6 +939,69 @@ describe('IndexedDbIndexManager', async () => {
await verifyResults(testingQuery, 'coll/val2');
});

it('can have filters on the same field', async () => {
await indexManager.addFieldIndex(
fieldIndex('coll', { fields: [['a', IndexKind.ASCENDING]] })
);
await indexManager.addFieldIndex(
fieldIndex('coll', {
fields: [
['a', IndexKind.ASCENDING],
['b', IndexKind.ASCENDING]
]
})
);
await addDoc('coll/val1', { 'a': 1, 'b': 1 });
await addDoc('coll/val2', { 'a': 2, 'b': 2 });
await addDoc('coll/val3', { 'a': 3, 'b': 3 });
await addDoc('coll/val4', { 'a': 4, 'b': 4 });

let testingQuery = queryWithAddedFilter(
queryWithAddedFilter(query('coll'), filter('a', '>', 1)),
filter('a', '==', 2)
);
await verifyResults(testingQuery, 'coll/val2');

testingQuery = queryWithAddedFilter(
queryWithAddedFilter(query('coll'), filter('a', '<=', 1)),
filter('a', '==', 2)
);
await verifyResults(testingQuery);

testingQuery = queryWithAddedOrderBy(
queryWithAddedFilter(
queryWithAddedFilter(query('coll'), filter('a', '>', 1)),
filter('a', '==', 2)
),
orderBy('a')
);
await verifyResults(testingQuery, 'coll/val2');

testingQuery = queryWithAddedOrderBy(
queryWithAddedOrderBy(
queryWithAddedFilter(
queryWithAddedFilter(query('coll'), filter('a', '>', 1)),
filter('a', '==', 2)
),
orderBy('a')
),
orderBy('__name__', 'desc')
);
await verifyResults(testingQuery, 'coll/val2');

testingQuery = queryWithAddedOrderBy(
queryWithAddedOrderBy(
queryWithAddedFilter(
queryWithAddedFilter(query('coll'), filter('a', '>', 1)),
filter('a', '==', 3)
),
orderBy('a')
),
orderBy('b', 'desc')
);
await verifyResults(testingQuery, 'coll/val3');
});

it('support advances queries', async () => {
// This test compares local query results with those received from the Java
// Server SDK.
Expand Down
74 changes: 74 additions & 0 deletions packages/firestore/test/unit/model/target_index_matcher.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -639,6 +639,80 @@ describe('Target Bounds', () => {
validateServesTarget(q, 'a', IndexKind.ASCENDING);
});

it('with equality and inequality on the same field', () => {
let q = queryWithAddedFilter(
queryWithAddedFilter(query('collId'), filter('a', '>=', 5)),
filter('a', '==', 0)
);
validateServesTarget(q, 'a', IndexKind.ASCENDING);

q = queryWithAddedOrderBy(
queryWithAddedFilter(
queryWithAddedFilter(query('collId'), filter('a', '>=', 5)),
filter('a', '==', 0)
),
orderBy('a')
);
validateServesTarget(q, 'a', IndexKind.ASCENDING);

q = queryWithAddedOrderBy(
queryWithAddedOrderBy(
queryWithAddedFilter(
queryWithAddedFilter(query('collId'), filter('a', '>=', 5)),
filter('a', '==', 0)
),
orderBy('a')
),
orderBy('__name__')
);
validateServesTarget(q, 'a', IndexKind.ASCENDING);

q = queryWithAddedOrderBy(
queryWithAddedOrderBy(
queryWithAddedFilter(
queryWithAddedFilter(query('collId'), filter('a', '>=', 5)),
filter('a', '==', 0)
),
orderBy('a')
),
orderBy('__name__', 'desc')
);
validateServesTarget(q, 'a', IndexKind.ASCENDING);

q = queryWithAddedOrderBy(
queryWithAddedOrderBy(
queryWithAddedOrderBy(
queryWithAddedFilter(
queryWithAddedFilter(query('collId'), filter('a', '>=', 5)),
filter('a', '==', 0)
),
orderBy('a')
),
orderBy('b')
),
orderBy('__name__', 'desc')
);
validateServesTarget(
q,
'a',
IndexKind.ASCENDING,
'b',
IndexKind.DESCENDING
);

q = queryWithAddedOrderBy(
queryWithAddedOrderBy(
queryWithAddedFilter(
queryWithAddedFilter(query('collId'), filter('a', '>=', 5)),
filter('a', '==', 0)
),
orderBy('a')
),
orderBy('__name__', 'desc')
);
validateServesTarget(q, 'a', IndexKind.ASCENDING);
});

function validateServesTarget(
query: Query,
field: string,
Expand Down