From 04358df1ad7e1b5717ae2fd51c45a4d1a7544bf8 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 4 Jul 2023 16:42:29 -0400 Subject: [PATCH 01/30] initial code --- packages/firestore/src/core/query.ts | 88 +++-- packages/firestore/src/lite-api/query.ts | 117 ++++--- .../src/model/target_index_matcher.ts | 2 +- .../test/integration/api/database.test.ts | 41 ++- .../test/integration/api/fields.test.ts | 44 +++ .../test/integration/api/query.test.ts | 25 +- .../test/integration/api/validation.test.ts | 327 ++++++++++-------- .../firestore/test/unit/core/query.test.ts | 47 ++- 8 files changed, 456 insertions(+), 235 deletions(-) diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index 9045a470e2c..70d7330052e 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -171,6 +171,7 @@ export function getFirstOrderByField(query: Query): FieldPath | null { : null; } +//Mila export function getInequalityFilterField(query: Query): FieldPath | null { for (const filter of query.filters) { const result = filter.getFirstInequalityField(); @@ -182,6 +183,18 @@ export function getInequalityFilterField(query: Query): FieldPath | null { return null; } +export function getInequalityFilterFields(query: Query): FieldPath[] | null { + const result: FieldPath[] = []; + for (const filter of query.filters) { + const field = filter.getFirstInequalityField(); + if (field !== null) { + result.push(field); + } + } + + return result.length === 0 ? null : result; +} + /** * Creates a new Query for a collection group query that matches all documents * within the provided collection group. @@ -216,29 +229,56 @@ export function isCollectionGroupQuery(query: Query): boolean { * the SDK and backend always orders by `__name__`). */ export function queryOrderBy(query: Query): OrderBy[] { + console.log("") const queryImpl = debugCast(query, QueryImpl); + if (queryImpl.memoizedOrderBy === null) { queryImpl.memoizedOrderBy = []; + console.log("before: ",JSON.stringify(queryImpl.memoizedOrderBy)); + + // //Note: Mila + // const inequalityFields = getInequalityFilterFields(queryImpl); + // const firstOrderByField = getFirstOrderByField(queryImpl); + // console.log("inequalityFields: ",JSON.stringify(inequalityFields)); + // console.log("firstOrderByField: ",JSON.stringify(firstOrderByField)); + + // if (inequalityFields !== null && firstOrderByField === null) { + // for (let inequalityField of inequalityFields) { + // // In order to implicitly add key ordering, we must also add the + // // inequality filter field for it to be a valid query. + // // Note that the default inequality field and key ordering is ascending. + // if (!inequalityField.isKeyField()) { + // queryImpl.memoizedOrderBy.push(new OrderBy(inequalityField)); + // } else { + // queryImpl.memoizedOrderBy.push( + // new OrderBy(FieldPath.keyField(), Direction.ASCENDING) + // ); } + + // } + // console.log("after: ",JSON.stringify(queryImpl.memoizedOrderBy)); const inequalityField = getInequalityFilterField(queryImpl); const firstOrderByField = getFirstOrderByField(queryImpl); if (inequalityField !== null && firstOrderByField === null) { - // In order to implicitly add key ordering, we must also add the - // inequality filter field for it to be a valid query. - // Note that the default inequality field and key ordering is ascending. - if (!inequalityField.isKeyField()) { - queryImpl.memoizedOrderBy.push(new OrderBy(inequalityField)); - } - queryImpl.memoizedOrderBy.push( - new OrderBy(FieldPath.keyField(), Direction.ASCENDING) - ); + // In order to implicitly add key ordering, we must also add the + // inequality filter field for it to be a valid query. + // Note that the default inequality field and key ordering is ascending. + if (!inequalityField.isKeyField()) { + queryImpl.memoizedOrderBy.push(new OrderBy(inequalityField)); + } + queryImpl.memoizedOrderBy.push( + new OrderBy(FieldPath.keyField(), Direction.ASCENDING) + ); + console.log(JSON.stringify(queryImpl.memoizedOrderBy)); + } else { - debugAssert( - inequalityField === null || - (firstOrderByField !== null && - inequalityField.isEqual(firstOrderByField)), - 'First orderBy should match inequality field.' - ); + //Note: Mila + // debugAssert( + // inequalityField === null || + // (firstOrderByField !== null && + // inequalityField.isEqual(firstOrderByField)), + // 'First orderBy should match inequality field.' + // ); let foundKeyOrdering = false; for (const orderBy of queryImpl.explicitOrderBy) { queryImpl.memoizedOrderBy.push(orderBy); @@ -314,15 +354,15 @@ export function queryToTarget(query: Query): Target { } export function queryWithAddedFilter(query: Query, filter: Filter): Query { - const newInequalityField = filter.getFirstInequalityField(); - const queryInequalityField = getInequalityFilterField(query); - - debugAssert( - queryInequalityField == null || - newInequalityField == null || - newInequalityField.isEqual(queryInequalityField), - 'Query must only have one inequality field.' - ); + // Note:Mila + // const newInequalityField = filter.getFirstInequalityField(); + // const queryInequalityField = getInequalityFilterField(query); + // debugAssert( + // queryInequalityField == null || + // newInequalityField == null || + // newInequalityField.isEqual(queryInequalityField), + // 'Query must only have one inequality field.' + // ); debugAssert( !isDocumentQuery(query), diff --git a/packages/firestore/src/lite-api/query.ts b/packages/firestore/src/lite-api/query.ts index 3afa282048f..aefa75b599f 100644 --- a/packages/firestore/src/lite-api/query.ts +++ b/packages/firestore/src/lite-api/query.ts @@ -845,7 +845,8 @@ export function newQueryOrderBy( ); } const orderBy = new OrderBy(fieldPath, direction); - validateNewOrderBy(query, orderBy); + // Mila + // validateNewOrderBy(query, orderBy); return orderBy; } @@ -1056,8 +1057,9 @@ function validateDisjunctiveFilterElements( */ function conflictingOps(op: Operator): Operator[] { switch (op) { + // Note: Mila case Operator.NOT_EQUAL: - return [Operator.NOT_EQUAL, Operator.NOT_IN]; + return [ /* Operator.NOT_EQUAL, */ Operator.NOT_IN]; case Operator.ARRAY_CONTAINS_ANY: case Operator.IN: return [Operator.NOT_IN]; @@ -1065,7 +1067,7 @@ function conflictingOps(op: Operator): Operator[] { return [ Operator.ARRAY_CONTAINS_ANY, Operator.IN, - Operator.NOT_IN, + // /* Operator.NOT_IN, */ Operator.NOT_EQUAL ]; default: @@ -1077,33 +1079,35 @@ function validateNewFieldFilter( query: InternalQuery, fieldFilter: FieldFilter ): void { - if (fieldFilter.isInequality()) { - const existingInequality = getInequalityFilterField(query); - const newInequality = fieldFilter.field; - - if ( - existingInequality !== null && - !existingInequality.isEqual(newInequality) - ) { - throw new FirestoreError( - Code.INVALID_ARGUMENT, - 'Invalid query. All where filters with an inequality' + - ' (<, <=, !=, not-in, >, or >=) must be on the same field. But you have' + - ` inequality filters on '${existingInequality.toString()}'` + - ` and '${newInequality.toString()}'` - ); - } - - const firstOrderByField = getFirstOrderByField(query); - if (firstOrderByField !== null) { - validateOrderByAndInequalityMatch( - query, - newInequality, - firstOrderByField - ); - } - } - + // Note: Mila + // if (fieldFilter.isInequality()) { + // const existingInequality = getInequalityFilterField(query); + // const newInequality = fieldFilter.field; + + // if ( + // existingInequality !== null && + // !existingInequality.isEqual(newInequality) + // ) { + // throw new FirestoreError( + // Code.INVALID_ARGUMENT, + // 'Invalid query. All where filters with an inequality' + + // ' (<, <=, !=, not-in, >, or >=) must be on the same field. But you have' + + // ` inequality filters on '${existingInequality.toString()}'` + + // ` and '${newInequality.toString()}'` + // ); + // } + + // const firstOrderByField = getFirstOrderByField(query); + // if (firstOrderByField !== null) { + // validateOrderByAndInequalityMatch( + // query, + // newInequality, + // firstOrderByField + // ); + // } + // } + + // Note:Mila const conflictingOp = findOpInsideFilters( query.filters, conflictingOps(fieldFilter.op) @@ -1151,32 +1155,33 @@ function findOpInsideFilters( return null; } -function validateNewOrderBy(query: InternalQuery, orderBy: OrderBy): void { - if (getFirstOrderByField(query) === null) { - // This is the first order by. It must match any inequality. - const inequalityField = getInequalityFilterField(query); - if (inequalityField !== null) { - validateOrderByAndInequalityMatch(query, inequalityField, orderBy.field); - } - } -} - -function validateOrderByAndInequalityMatch( - baseQuery: InternalQuery, - inequality: InternalFieldPath, - orderBy: InternalFieldPath -): void { - if (!orderBy.isEqual(inequality)) { - throw new FirestoreError( - Code.INVALID_ARGUMENT, - `Invalid query. You have a where filter with an inequality ` + - `(<, <=, !=, not-in, >, or >=) on field '${inequality.toString()}' ` + - `and so you must also use '${inequality.toString()}' ` + - `as your first argument to orderBy(), but your first orderBy() ` + - `is on field '${orderBy.toString()}' instead.` - ); - } -} +// function validateNewOrderBy(query: InternalQuery, orderBy: OrderBy): void { +// if (getFirstOrderByField(query) === null) { +// // This is the first order by. It must match any inequality. +// const inequalityField = getInequalityFilterField(query); +// if (inequalityField !== null) { +// validateOrderByAndInequalityMatch(query, inequalityField, orderBy.field); +// } +// } +// } + +// function validateOrderByAndInequalityMatch( +// baseQuery: InternalQuery, +// inequality: InternalFieldPath, +// orderBy: InternalFieldPath +// ): void { +// if (!orderBy.isEqual(inequality)) { +// // Note:Mila +// throw new FirestoreError( +// Code.INVALID_ARGUMENT, +// `Invalid query. You have a where filter with an inequality ` + +// `(<, <=, !=, not-in, >, or >=) on field '${inequality.toString()}' ` + +// `and so you must also use '${inequality.toString()}' ` + +// `as your first argument to orderBy(), but your first orderBy() ` + +// `is on field '${orderBy.toString()}' instead.` +// ); +// } +// } export function validateQueryFilterConstraint( functionName: string, diff --git a/packages/firestore/src/model/target_index_matcher.ts b/packages/firestore/src/model/target_index_matcher.ts index d3bd95d473c..1dd6c6589d3 100644 --- a/packages/firestore/src/model/target_index_matcher.ts +++ b/packages/firestore/src/model/target_index_matcher.ts @@ -65,7 +65,7 @@ export class TargetIndexMatcher { : target.path.lastSegment(); this.orderBys = target.orderBy; this.equalityFilters = []; - + // Note:Mila for (const filter of target.filters) { const fieldFilter = filter as FieldFilter; if (fieldFilter.isInequality()) { diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 0b22720d67a..da563f00824 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -78,6 +78,7 @@ import { withEnsuredEagerGcTestDb } from '../util/helpers'; import { DEFAULT_SETTINGS, DEFAULT_PROJECT_ID } from '../util/settings'; +import { ref } from '../../util/helpers'; use(chaiAsPromised); @@ -641,34 +642,42 @@ apiDescribe('Database', (persistence: boolean) => { apiDescribe('Queries are validated client-side', (persistence: boolean) => { // NOTE: Failure cases are validated in validation_test.ts - it('same inequality fields works', () => { + it('inequality and equality on different fields works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => - query(coll, where('x', '>=', 32), where('x', '<=', 'cat')) + query(coll, where('x', '>=', 32), where('y', '==', 'cat')) ).not.to.throw(); }); }); - it('inequality and equality on different fields works', () => { + it('inequality on same field works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => - query(coll, where('x', '>=', 32), where('y', '==', 'cat')) + query(coll, where('x', '>=', 32), where('x', '<', '42')) ).not.to.throw(); }); }); - it('inequality and array-contains on different fields works', () => { + it('inequality on multiple fields works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => - query(coll, where('x', '>=', 32), where('y', 'array-contains', 'cat')) + query(coll, where('x', '>=', 32), where('y', '<=', 'cat')) ).not.to.throw(); }); }); - it('inequality and IN on different fields works', () => { + it('multiple inequality with key fields works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => - query(coll, where('x', '>=', 32), where('y', 'in', [1, 2])) + query(coll, where(documentId(), ">=", ref('collection/id')), where('x', '>=', 32)) + ).not.to.throw(); + }); + }); + + it('inequality and array-contains on different fields works', () => { + return withTestCollection(persistence, {}, async coll => { + expect(() => + query(coll, where('x', '>=', 32), where('y', 'array-contains', 'cat')) ).not.to.throw(); }); }); @@ -685,6 +694,22 @@ apiDescribe('Database', (persistence: boolean) => { }); }); + it('inequality and IN on different fields works', () => { + return withTestCollection(persistence, {}, async coll => { + expect(() => + query(coll, where('x', '>=', 32), where('y', 'in', [1, 2])) + ).not.to.throw(); + }); + }); + + it('inequality and NOT IN on different fields works', () => { + return withTestCollection(persistence, {}, async coll => { + expect(() => + query(coll, where('x', '>=', 32), where('y', 'not-in', [1, 2])) + ).not.to.throw(); + }); + }); + it('inequality same as orderBy works.', () => { return withTestCollection(persistence, {}, async coll => { expect(() => diff --git a/packages/firestore/test/integration/api/fields.test.ts b/packages/firestore/test/integration/api/fields.test.ts index 0456ad5e48d..de87254ca70 100644 --- a/packages/firestore/test/integration/api/fields.test.ts +++ b/packages/firestore/test/integration/api/fields.test.ts @@ -179,6 +179,30 @@ apiDescribe('Nested Fields', (persistence: boolean) => { }); }); + +// it.only('can be used with query.where().', () => { +// const testDocs = { +// '1': testData(100), +// '2': testData(200), +// '3': testData(500), +// '4': testData(300), + + +// }; +// return withTestCollection(persistence, testDocs, coll => { +// return getDocs(query(coll, where('metadata.createdAt', '>=', 200), where('name','!=','room 200'), orderBy('name'))).then( +// results => { +// // inequality adds implicit sort on field +// expect(toDataArray(results)).to.deep.equal([ +// testData(300), +// testData(500) + +// ]); +// } +// ); +// }); +// }); + it('can be used with query.where().', () => { const testDocs = { '1': testData(300), @@ -320,6 +344,26 @@ apiDescribe('Fields with special characters', (persistence: boolean) => { }); }); + it('can be used in multiple inequality query filters.', () => { + const testDocs = { + '1': testData(300), + '2': testData(100), + '3': testData(200) + }; + return withTestCollection(persistence, testDocs, coll => { + // inequality adds implicit sort on field + const expected = [testData(200)]; + return getDocs(query(coll, where('field', '>=', 'field 200'), where(new FieldPath('field.dot'), '!=', 300))) + .then(results => { + expect(toDataArray(results)).to.deep.equal(expected); + }) + .then(() => getDocs(query(coll, where('field', '<=', 'field 200'), where('field\\slash', '>=', 200)))) + .then(results => { + expect(toDataArray(results)).to.deep.equal(expected); + }); + }); + }); + it('can be used in a query orderBy.', () => { const testDocs = { '1': testData(300), diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index bb2f35a6365..1813264e1e4 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -714,15 +714,16 @@ apiDescribe('Queries', (persistence: boolean) => { it('can query by document ID', () => { const testDocs = { - aa: { key: 'aa' }, - ab: { key: 'ab' }, - ba: { key: 'ba' }, - bb: { key: 'bb' } + aa: { key: '1' }, + ab: { key: '2' }, + ba: { key: '3' }, + bb: { key: '4' } }; return withTestCollection(persistence, testDocs, coll => { return getDocs(query(coll, where(documentId(), '==', 'ab'))) .then(docs => { expect(toDataArray(docs)).to.deep.equal([testDocs['ab']]); + console.log("===========1") return getDocs( query( coll, @@ -732,6 +733,8 @@ apiDescribe('Queries', (persistence: boolean) => { ); }) .then(docs => { + console.log("===========2") + expect(toDataArray(docs)).to.deep.equal([ testDocs['ab'], testDocs['ba'] @@ -1690,13 +1693,21 @@ apiDescribe('Queries', (persistence: boolean) => { 'doc3' ); - // Test with limits (implicit order by ASC): (a==1) || (b > 0) LIMIT 2 + // with multiple inequality: a>2 || b<=1. await checkOnlineAndOfflineResultsMatch( - query(coll, or(where('a', '==', 1), where('b', '>', 0)), limit(2)), + query(coll, or(where('a', '>', 2), where('b', '<', 1))), 'doc1', - 'doc2' + 'doc3', ); + // Mila: why this fail + // // Test with limits (implicit order by ASC): (a==1) || (b > 0) LIMIT 2 + // await checkOnlineAndOfflineResultsMatch( + // query(coll, or(where('a', '==', 1), where('b', '>', 0)), limit(2)), + // 'doc1', + // 'doc2' + // ); + // Test with limits (explicit order by): (a==1) || (b > 0) LIMIT_TO_LAST 2 // Note: The public query API does not allow implicit ordering when limitToLast is used. await checkOnlineAndOfflineResultsMatch( diff --git a/packages/firestore/test/integration/api/validation.test.ts b/packages/firestore/test/integration/api/validation.test.ts index f422eb4ad07..3af317fd33f 100644 --- a/packages/firestore/test/integration/api/validation.test.ts +++ b/packages/firestore/test/integration/api/validation.test.ts @@ -816,82 +816,6 @@ apiDescribe('Validation:', (persistence: boolean) => { } ); - validationIt(persistence, 'with different inequality fields fail', db => { - const coll = collection(db, 'test'); - expect(() => - query(coll, where('x', '>=', 32), where('y', '<', 'cat')) - ).to.throw( - 'Invalid query. All where filters with an ' + - 'inequality (<, <=, !=, not-in, >, or >=) must be on the same field.' + - ` But you have inequality filters on 'x' and 'y'` - ); - }); - - validationIt(persistence, 'with more than one != query fail', db => { - const coll = collection(db, 'test'); - expect(() => - query(coll, where('x', '!=', 32), where('x', '!=', 33)) - ).to.throw("Invalid query. You cannot use more than one '!=' filter."); - }); - - validationIt( - persistence, - 'with != and inequality queries on different fields fail', - db => { - const coll = collection(db, 'test'); - expect(() => - query(coll, where('y', '>', 32), where('x', '!=', 33)) - ).to.throw( - 'Invalid query. All where filters with an ' + - 'inequality (<, <=, !=, not-in, >, or >=) must be on the same field.' + - ` But you have inequality filters on 'y' and 'x` - ); - } - ); - - validationIt( - persistence, - 'with != and inequality queries on different fields fail', - db => { - const coll = collection(db, 'test'); - expect(() => - query(coll, where('y', '>', 32), where('x', 'not-in', [33])) - ).to.throw( - 'Invalid query. All where filters with an ' + - 'inequality (<, <=, !=, not-in, >, or >=) must be on the same field.' + - ` But you have inequality filters on 'y' and 'x` - ); - } - ); - - validationIt( - persistence, - 'with inequality different than first orderBy fail.', - db => { - const coll = collection(db, 'test'); - const reason = - `Invalid query. You have a where filter with an ` + - `inequality (<, <=, !=, not-in, >, or >=) on field 'x' and so you must also ` + - `use 'x' as your first argument to orderBy(), but your first ` + - `orderBy() is on field 'y' instead.`; - expect(() => query(coll, where('x', '>', 32), orderBy('y'))).to.throw( - reason - ); - expect(() => query(coll, orderBy('y'), where('x', '>', 32))).to.throw( - reason - ); - expect(() => - query(coll, where('x', '>', 32), orderBy('y'), orderBy('x')) - ).to.throw(reason); - expect(() => - query(coll, orderBy('y'), orderBy('x'), where('x', '>', 32)) - ).to.throw(reason); - expect(() => query(coll, where('x', '!=', 32), orderBy('y'))).to.throw( - reason - ); - } - ); - validationIt(persistence, 'with != and not-in filters fail', db => { expect(() => query( @@ -915,15 +839,16 @@ apiDescribe('Validation:', (persistence: boolean) => { }); validationIt(persistence, 'with multiple disjunctive filters fail', db => { - expect(() => - query( - collection(db, 'test'), - where('foo', 'not-in', [1, 2]), - where('foo', 'not-in', [2, 3]) - ) - ).to.throw( - "Invalid query. You cannot use more than one 'not-in' filter." - ); + // Mila: is this multiple inequality? + // expect(() => + // query( + // collection(db, 'test'), + // where('foo', 'not-in', [1, 2]), + // where('foo', 'not-in', [2, 3]) + // ) + // ).to.throw( + // "Invalid query. You cannot use more than one 'not-in' filter." + // ); expect(() => query( @@ -1139,36 +1064,7 @@ apiDescribe('Validation:', (persistence: boolean) => { }); validationIt(persistence, 'invalid query filters fail', db => { - // Multiple inequalities, one of which is inside a nested composite filter. const coll = collection(db, 'test'); - expect(() => - query( - coll, - and( - or( - and(where('a', '==', 'b'), where('c', '>', 'd')), - and(where('e', '==', 'f'), where('g', '==', 'h')) - ), - where('r', '>', 's') - ) - ) - ).to.throw( - "Invalid query. All where filters with an inequality (<, <=, !=, not-in, >, or >=) must be on the same field. But you have inequality filters on 'c' and 'r'" - ); - - // OrderBy and inequality on different fields. Inequality inside a nested composite filter. - expect(() => - query( - coll, - or( - and(where('a', '==', 'b'), where('c', '>', 'd')), - and(where('e', '==', 'f'), where('g', '==', 'h')) - ), - orderBy('r') - ) - ).to.throw( - "Invalid query. You have a where filter with an inequality (<, <=, !=, not-in, >, or >=) on field 'c' and so you must also use 'c' as your first argument to orderBy(), but your first orderBy() is on field 'r' instead." - ); // Conflicting operations within a composite filter. expect(() => @@ -1217,30 +1113,6 @@ apiDescribe('Validation:', (persistence: boolean) => { ).to.throw( "Invalid query. You cannot use 'not-in' filters with 'in' filters." ); - - // Multiple top level composite filters - expect(() => - // @ts-ignore - query(coll, and(where('a', '==', 'b')), or(where('b', '==', 'a'))) - ).to.throw( - 'InvalidQuery. When using composite filters, you cannot use ' + - 'more than one filter at the top level. Consider nesting the multiple ' + - 'filters within an `and(...)` statement. For example: ' + - 'change `query(query, where(...), or(...))` to ' + - '`query(query, and(where(...), or(...)))`.' - ); - - // Once top level composite filter and one top level field filter - expect(() => - // @ts-ignore - query(coll, or(where('a', '==', 'b')), where('b', '==', 'a')) - ).to.throw( - 'InvalidQuery. When using composite filters, you cannot use ' + - 'more than one filter at the top level. Consider nesting the multiple ' + - 'filters within an `and(...)` statement. For example: ' + - 'change `query(query, where(...), or(...))` to ' + - '`query(query, and(where(...), or(...)))`.' - ); }); validationIt( @@ -1279,6 +1151,185 @@ apiDescribe('Validation:', (persistence: boolean) => { } } ); + + // Multiple Inequality validation tests + validationIt(persistence, 'can have multiple inequality fields', db => { + const coll = collection(db, 'test'); + expect(() => + query(coll, where('x', '>=', 32), where('y', '<', 42)) + ).not.to.throw(); + }); + + validationIt(persistence, 'can have more than one !=', db => { + const coll = collection(db, 'test'); + expect(() => + query(coll, where('x', '!=', 32), where('y', '!=', 42)) + ).not.to.throw(); + }); + + validationIt(persistence, 'can have more than one not-in', db => { + const coll = collection(db, 'test'); + expect(() => + query(coll, where('x', 'not-in', [32]), where('y', 'not-in', [42])) + ).not.to.throw(); + }); + + validationIt( + persistence, + 'can have != and inequality queries on different fields', + db => { + const coll = collection(db, 'test'); + expect(() => + query(coll, where('x', '>', 32), where('y', '!=', 42)) + ).not.to.throw(); + } + ); + + validationIt( + persistence, + 'can have not-in and inequality queries on different fields', + db => { + const coll = collection(db, 'test'); + expect(() => + query(coll, where('y', '>=', 32), where('x', 'not-in', [42])) + ).not.to.throw(); + } + ); + + validationIt( + persistence, + 'can have inequality different than first orderBy', + db => { + const coll = collection(db, 'test'); + // single inequality + expect(() => query(coll, where('x', '>', 32), orderBy('y'))).not.to.throw(); + expect(() => query(coll, orderBy('y'), where('x', '>', 32))).not.to.throw(); + expect(() => + query(coll, where('x', '>', 32), orderBy('y'), orderBy('x')) + ).not.to.throw(); + expect(() => + query(coll, orderBy('y'), orderBy('x'), where('x', '>', 32)) + ).not.to.throw(); + expect(() => query(coll, where('x', '!=', 32), orderBy('y'))).not.to.throw(); + + // multiple inequality + expect(() => query(coll, where('x', '>', 32), where('y', '!=', 42), orderBy('z'))).not.to.throw(); + expect(() => query(coll, orderBy('y'), where('x', '>', 32), where('y', '<=', 42))).not.to.throw(); + expect(() => + query(coll, where('x', '>', 32), where('y', '!=', 42), orderBy('y'), orderBy('x')) + ).not.to.throw(); + expect(() => + query(coll, orderBy('y'), orderBy('z'), where('x', '>', 32), where('y', '!=', 42)) + ).not.to.throw(); + } + ); + + validationIt(persistence, 'multiple inequalities inside a nested composite filter', db => { + // Multiple inequality on different fields inside a nested composite filter. + const coll = collection(db, 'test'); + expect(() => + query( + coll, + or( + and(where('a', '==', 'b'), where('c', '>', 'd')), + and(where('e', '<=', 'f'), where('g', '==', 'h')) + ) + ) + ).not.to.throw(); + + // Multiple inequality on different fields between a field filter and a composite filter. + expect(() => + query( + coll, + and( + or( + and(where('a', '==', 'b'), where('c', '>=', 'd')), + and(where('e', '==', 'f'), where('g', '!=', 'h')) + ), + where('r', '<', 's') + ) + ) + ).not.to.throw(); + + // OrderBy and multiple inequality on different fields. + expect(() => + query( + coll, + or( + and(where('a', '==', 'b'), where('c', '>', 'd')), + and(where('e', '==', 'f'), where('g', '!=', 'h')) + ), + orderBy('r'), + orderBy('a') + ) + ).not.to.throw(); + + // Multiple inequality inside two composite filters. + expect(() => + query( + coll, + and( + or( + and(where('a', '==', 'b'), where('c', '>=', 'd')), + and(where('e', '==', 'f'), where('g', '!=', 'h')) + ), + or( + and(where('i', '==', 'j'), where('k', '!=', 'l')), + and(where('m', '==', 'n'), where('o', '<', 'p')) + ) + ) + ) + ).not.to.throw(); + + // Multiple top level composite filters + expect(() => + // @ts-ignore + query(coll, and(where('a', '==', 'b')), or(where('b', '==', 'a'))) + ).to.throw( + 'InvalidQuery. When using composite filters, you cannot use ' + + 'more than one filter at the top level. Consider nesting the multiple ' + + 'filters within an `and(...)` statement. For example: ' + + 'change `query(query, where(...), or(...))` to ' + + '`query(query, and(where(...), or(...)))`.' + ); + + // Once top level composite filter and one top level field filter + expect(() => + // @ts-ignore + query(coll, or(where('a', '==', 'b')), where('b', '==', 'a')) + ).to.throw( + 'InvalidQuery. When using composite filters, you cannot use ' + + 'more than one filter at the top level. Consider nesting the multiple ' + + 'filters within an `and(...)` statement. For example: ' + + 'change `query(query, where(...), or(...))` to ' + + '`query(query, and(where(...), or(...)))`.' + ); + + // Multiple top level composite filters + expect(() => + // @ts-ignore + query(coll, and(where('a', '==', 'b')), or(where('b', '==', 'a'))) + ).to.throw( + 'InvalidQuery. When using composite filters, you cannot use ' + + 'more than one filter at the top level. Consider nesting the multiple ' + + 'filters within an `and(...)` statement. For example: ' + + 'change `query(query, where(...), or(...))` to ' + + '`query(query, and(where(...), or(...)))`.' + ); + + // Once top level composite filter and one top level field filter + expect(() => + // @ts-ignore + query(coll, or(where('a', '==', 'b')), where('b', '==', 'a')) + ).to.throw( + 'InvalidQuery. When using composite filters, you cannot use ' + + 'more than one filter at the top level. Consider nesting the multiple ' + + 'filters within an `and(...)` statement. For example: ' + + 'change `query(query, where(...), or(...))` to ' + + '`query(query, and(where(...), or(...)))`.' + ); + }); + }); }); diff --git a/packages/firestore/test/unit/core/query.test.ts b/packages/firestore/test/unit/core/query.test.ts index c88532474be..064f6214a77 100644 --- a/packages/firestore/test/unit/core/query.test.ts +++ b/packages/firestore/test/unit/core/query.test.ts @@ -820,7 +820,7 @@ describe('Query', () => { orFilter(filter('a', '>', 2), filter('b', '==', 1)) ); assertQueryMatches(query2, [doc2, doc3, doc5], [doc1, doc4]); - + // (a==1 && b==0) || (a==3 && b==2) const query3 = query( 'collection', @@ -852,6 +852,51 @@ describe('Query', () => { assertQueryMatches(query5, [doc3], [doc1, doc2, doc4, doc5]); }); + it('matches composite queries with multiple inequality', () => { + const doc1 = doc('collection/1', 0, { a: 1, b: 0 }); + const doc2 = doc('collection/2', 0, { a: 2, b: 1 }); + const doc3 = doc('collection/3', 0, { a: 3, b: 2 }); + const doc4 = doc('collection/4', 0, { a: 1, b: 3 }); + const doc5 = doc('collection/5', 0, { a: 1, b: 1 }); + + // a>1 || b!=1. + const query1 = query( + 'collection', + orFilter(filter('a', '>', 1), filter('b', '!=', 1)) + ); + assertQueryMatches(query1, [doc1, doc2, doc3, doc4], [doc5]); + + // (a>=1 && b==0) || (a==1 && b!=1) + const query2 = query( + 'collection', + orFilter( + andFilter(filter('a', '>=', 1), filter('b', '==', 0)), + andFilter(filter('a', '==', 1), filter('b', '!=', 1)) + ) + ); + assertQueryMatches(query2, [doc1, doc4], [doc2, doc3, doc5]); + + // a<=2 && (a!=1 || b<3). + const query3 = query( + 'collection', + andFilter( + filter('a', '<=', 2), + orFilter(filter('a', '!=', 1), filter('b', '<', 3)) + ) + ); + assertQueryMatches(query3, [doc1, doc2, doc5], [doc3, doc4]); + + // (a!=1 || b!=1) && (a>=3 || b<3) + const query5 = query( + 'collection', + andFilter( + orFilter(filter('a', '!=', 1), filter('b', '!=', 1)), + orFilter(filter('a', '>=', 3), filter('b', '<', 3)) + ) + ); + assertQueryMatches(query5, [doc1, doc2, doc3 ], [ doc4, doc5]); + }); + function assertQueryMatches( query: Query, matching: MutableDocument[], From 31d5c9d2bdd8e17205558d5dcb1a232527d9a3f2 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 5 Jul 2023 19:23:51 -0400 Subject: [PATCH 02/30] remove validation in TargetIndexMatcher --- packages/firestore/src/core/query.ts | 25 ----- packages/firestore/src/lite-api/query.ts | 5 +- .../src/model/target_index_matcher.ts | 25 ++--- .../test/integration/api/database.test.ts | 26 +++++- .../test/integration/api/fields.test.ts | 91 +++++++++++-------- .../test/integration/api/query.test.ts | 52 ++++++++++- .../test/integration/api/validation.test.ts | 42 ++++----- 7 files changed, 158 insertions(+), 108 deletions(-) diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index 70d7330052e..1c67af211f4 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -229,34 +229,11 @@ export function isCollectionGroupQuery(query: Query): boolean { * the SDK and backend always orders by `__name__`). */ export function queryOrderBy(query: Query): OrderBy[] { - console.log("") const queryImpl = debugCast(query, QueryImpl); if (queryImpl.memoizedOrderBy === null) { queryImpl.memoizedOrderBy = []; - console.log("before: ",JSON.stringify(queryImpl.memoizedOrderBy)); - // //Note: Mila - // const inequalityFields = getInequalityFilterFields(queryImpl); - // const firstOrderByField = getFirstOrderByField(queryImpl); - // console.log("inequalityFields: ",JSON.stringify(inequalityFields)); - // console.log("firstOrderByField: ",JSON.stringify(firstOrderByField)); - - // if (inequalityFields !== null && firstOrderByField === null) { - // for (let inequalityField of inequalityFields) { - // // In order to implicitly add key ordering, we must also add the - // // inequality filter field for it to be a valid query. - // // Note that the default inequality field and key ordering is ascending. - // if (!inequalityField.isKeyField()) { - // queryImpl.memoizedOrderBy.push(new OrderBy(inequalityField)); - // } else { - // queryImpl.memoizedOrderBy.push( - // new OrderBy(FieldPath.keyField(), Direction.ASCENDING) - // ); } - - // } - // console.log("after: ",JSON.stringify(queryImpl.memoizedOrderBy)); - const inequalityField = getInequalityFilterField(queryImpl); const firstOrderByField = getFirstOrderByField(queryImpl); if (inequalityField !== null && firstOrderByField === null) { @@ -269,8 +246,6 @@ export function queryOrderBy(query: Query): OrderBy[] { queryImpl.memoizedOrderBy.push( new OrderBy(FieldPath.keyField(), Direction.ASCENDING) ); - console.log(JSON.stringify(queryImpl.memoizedOrderBy)); - } else { //Note: Mila // debugAssert( diff --git a/packages/firestore/src/lite-api/query.ts b/packages/firestore/src/lite-api/query.ts index 2173239f187..b7ed4f386d5 100644 --- a/packages/firestore/src/lite-api/query.ts +++ b/packages/firestore/src/lite-api/query.ts @@ -1080,9 +1080,8 @@ function validateDisjunctiveFilterElements( */ function conflictingOps(op: Operator): Operator[] { switch (op) { - // Note: Mila case Operator.NOT_EQUAL: - return [ /* Operator.NOT_EQUAL, */ Operator.NOT_IN]; + return [ Operator.NOT_EQUAL, Operator.NOT_IN]; case Operator.ARRAY_CONTAINS_ANY: case Operator.IN: return [Operator.NOT_IN]; @@ -1090,7 +1089,7 @@ function conflictingOps(op: Operator): Operator[] { return [ Operator.ARRAY_CONTAINS_ANY, Operator.IN, - // /* Operator.NOT_IN, */ + Operator.NOT_IN, Operator.NOT_EQUAL ]; default: diff --git a/packages/firestore/src/model/target_index_matcher.ts b/packages/firestore/src/model/target_index_matcher.ts index 1dd6c6589d3..f8763d3b2b9 100644 --- a/packages/firestore/src/model/target_index_matcher.ts +++ b/packages/firestore/src/model/target_index_matcher.ts @@ -27,6 +27,7 @@ import { IndexKind, IndexSegment } from './field_index'; +import { FieldPath } from './path'; /** * A light query planner for Firestore. @@ -51,8 +52,8 @@ import { export class TargetIndexMatcher { // The collection ID (or collection group) of the query target. private readonly collectionId: string; - // The single inequality filter of the target (if it exists). - private readonly inequalityFilter?: FieldFilter; + // The inequality filters of the target (if it exists). + private readonly inequalityFilters = new Map(); // The list of equality filters of the target. private readonly equalityFilters: FieldFilter[]; // The list of orderBys of the target. @@ -69,12 +70,7 @@ export class TargetIndexMatcher { for (const filter of target.filters) { const fieldFilter = filter as FieldFilter; if (fieldFilter.isInequality()) { - debugAssert( - !this.inequalityFilter || - this.inequalityFilter.field.isEqual(fieldFilter.field), - 'Only a single inequality is supported' - ); - this.inequalityFilter = fieldFilter; + this.inequalityFilters.set(fieldFilter.field.canonicalString(), fieldFilter); } else { this.equalityFilters.push(fieldFilter); } @@ -145,16 +141,23 @@ export class TargetIndexMatcher { return true; } - if (this.inequalityFilter !== undefined) { + if (this.inequalityFilters.size > 0) { + if (this.inequalityFilters.size > 1) { + // Only single inequality is supported now. + return false; + } + + const inequalityFilter = this.inequalityFilters.values().next().value; // 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 ( - !equalitySegments.has(this.inequalityFilter.field.canonicalString()) + !equalitySegments.has(inequalityFilter.field.canonicalString()) ) { const segment = segments[segmentIndex]; + if ( - !this.matchesFilter(this.inequalityFilter, segment) || + !this.matchesFilter(inequalityFilter, segment) || !this.matchesOrderBy(this.orderBys[orderBysIndex++], segment) ) { return false; diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index fd87b9f468e..dd08f9561d7 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -654,7 +654,7 @@ apiDescribe('Database', persistence => { it('inequality on same field works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => - query(coll, where('x', '>=', 32), where('x', '<', '42')) + query(coll, where('x', '>=', 32), where('x', '<', 42)) ).not.to.throw(); }); }); @@ -662,7 +662,7 @@ apiDescribe('Database', persistence => { it('inequality on multiple fields works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => - query(coll, where('x', '>=', 32), where('y', '<=', 'cat')) + query(coll, where('x', '>=', 32), where('y', '!=', 'cat')) ).not.to.throw(); }); }); @@ -763,6 +763,28 @@ apiDescribe('Database', persistence => { }); }); + it('inequality different from orderBy works.', () => { + return withTestCollection(persistence, {}, async coll => { + expect(() => + query(coll, where('x', '>', 32), orderBy('y')) + ).not.to.throw(); + expect(() => + query(coll, orderBy('y'), where('x', '>', 32)) + ).not.to.throw(); + }); + }); + + it('inequality different from first orderBy works.', () => { + return withTestCollection(persistence, {}, async coll => { + expect(() => + query(coll, where('x', '>', 32), orderBy('y'), orderBy('z')) + ).not.to.throw(); + expect(() => + query(coll, orderBy('y'), where('x', '>', 32), orderBy('x')) + ).not.to.throw(); + }); + }); + it('array-contains different than orderBy works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => diff --git a/packages/firestore/test/integration/api/fields.test.ts b/packages/firestore/test/integration/api/fields.test.ts index 81fcf194684..486a6c616a6 100644 --- a/packages/firestore/test/integration/api/fields.test.ts +++ b/packages/firestore/test/integration/api/fields.test.ts @@ -37,7 +37,7 @@ import { withTestDoc, withTestDocAndSettings } from '../util/helpers'; -import { DEFAULT_SETTINGS } from '../util/settings'; +import { DEFAULT_SETTINGS, USE_EMULATOR } from '../util/settings'; // Allow custom types for testing. // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -179,30 +179,6 @@ apiDescribe('Nested Fields', persistence => { }); }); - -// it.only('can be used with query.where().', () => { -// const testDocs = { -// '1': testData(100), -// '2': testData(200), -// '3': testData(500), -// '4': testData(300), - - -// }; -// return withTestCollection(persistence, testDocs, coll => { -// return getDocs(query(coll, where('metadata.createdAt', '>=', 200), where('name','!=','room 200'), orderBy('name'))).then( -// results => { -// // inequality adds implicit sort on field -// expect(toDataArray(results)).to.deep.equal([ -// testData(300), -// testData(500) - -// ]); -// } -// ); -// }); -// }); - it('can be used with query.where().', () => { const testDocs = { '1': testData(300), @@ -344,45 +320,82 @@ apiDescribe('Fields with special characters', persistence => { }); }); - it('can be used in multiple inequality query filters.', () => { + + + it('can be used in a query orderBy.', () => { const testDocs = { '1': testData(300), '2': testData(100), '3': testData(200) }; return withTestCollection(persistence, testDocs, coll => { - // inequality adds implicit sort on field - const expected = [testData(200)]; - return getDocs(query(coll, where('field', '>=', 'field 200'), where(new FieldPath('field.dot'), '!=', 300))) + const expected = [testData(100), testData(200), testData(300)]; + return getDocs(query(coll, orderBy(new FieldPath('field.dot')))) .then(results => { expect(toDataArray(results)).to.deep.equal(expected); }) - .then(() => getDocs(query(coll, where('field', '<=', 'field 200'), where('field\\slash', '>=', 200)))) + .then(() => getDocs(query(coll, orderBy('field\\slash')))) .then(results => { expect(toDataArray(results)).to.deep.equal(expected); }); }); }); +}); - it('can be used in a query orderBy.', () => { + // eslint-disable-next-line no-restricted-properties +(USE_EMULATOR ? apiDescribe : apiDescribe.skip)('Multiple Inequality', persistence => { + const testData = (n?: number): AnyTestData => { + n = n || 1; + return { + name: 'room ' + n, + metadata: { + createdAt: n, + }, + field: 'field ' + n, + 'field.dot': n, + 'field\\slash': n + }; + }; + it('can be used with query.where().', () => { const testDocs = { - '1': testData(300), + '1': testData(100), + '2': testData(200), + '3': testData(500), + '4': testData(300), + }; + return withTestCollection(persistence, testDocs, coll => { + return getDocs(query(coll, where('metadata.createdAt', '<=', 500), where('metadata.createdAt', '>', 100), where('name', '!=', 'room 200'), orderBy('name'))).then( + results => { + // inequality adds implicit sort on field + expect(toDataArray(results)).to.deep.equal([ + testData(300), + testData(500), + ]); + } + ); + }); + }); + + it('can be used in multiple inequality query filters.', () => { + const testDocs = { + '1': testData(400), '2': testData(100), - '3': testData(200) + '3': testData(200), + '4': testData(300), }; return withTestCollection(persistence, testDocs, coll => { - const expected = [testData(100), testData(200), testData(300)]; - return getDocs(query(coll, orderBy(new FieldPath('field.dot')))) + return getDocs(query(coll, where('field', '>=', 'field 200'), where(new FieldPath('field.dot'), '!=', 300))) .then(results => { - expect(toDataArray(results)).to.deep.equal(expected); + expect(toDataArray(results)).to.deep.equal([testData(200), testData(400)]); }) - .then(() => getDocs(query(coll, orderBy('field\\slash')))) + .then(() => getDocs(query(coll, where('field', '<=', 'field 200'), where('field\\slash', '>=', 200)))) .then(results => { - expect(toDataArray(results)).to.deep.equal(expected); + expect(toDataArray(results)).to.deep.equal([testData(200)]); }); }); }); -}); + }) + apiDescribe('Timestamp Fields in snapshots', persistence => { // Figure out how to pass in the Timestamp type diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 67f869d684a..40a9729191c 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -724,7 +724,6 @@ apiDescribe('Queries', persistence => { return getDocs(query(coll, where(documentId(), '==', 'ab'))) .then(docs => { expect(toDataArray(docs)).to.deep.equal([testDocs['ab']]); - console.log("===========1") return getDocs( query( coll, @@ -734,8 +733,6 @@ apiDescribe('Queries', persistence => { ); }) .then(docs => { - console.log("===========2") - expect(toDataArray(docs)).to.deep.equal([ testDocs['ab'], testDocs['ba'] @@ -1337,6 +1334,55 @@ apiDescribe('Queries', persistence => { }); }); + // eslint-disable-next-line no-restricted-properties + ( USE_EMULATOR ? describe : describe.skip)( + 'OR Queries', + () => { + it('key order is descending for descending multiple inequality', () => { + const testDocs = { + a: { + foo: 42, + bar: 'ab' + }, + b: { + foo: 42.0, + bar: 'aa' + }, + c: { + foo: 42, + bar: 'ba' + }, + d: { + foo: 21, + bar: 'b' + }, + e: { + foo: 21, + bar: 'a' + }, + f: { + foo: 66, + bar: 'ac' + }, + g: { + foo: 66, + bar: 'c' + } + }; + return withTestCollection(persistence, testDocs, coll => { + return getDocs( + query(coll, where('foo', '>', 21.0),where('bar', 'not-in', ['a','ac','ba']), orderBy('foo', 'desc') ) + ).then(docs => { + expect(docs.docs.map(d => d.id)).to.deep.equal([ + 'g', 'b', 'a' + ]); + }); + }); + }); + + } + ); + // OR Query tests only run when the SDK's local cache is configured to use // LRU garbage collection (rather than eager garbage collection) because // they validate that the result from server and cache match. diff --git a/packages/firestore/test/integration/api/validation.test.ts b/packages/firestore/test/integration/api/validation.test.ts index 8cd2ed6c6bd..e130695b40f 100644 --- a/packages/firestore/test/integration/api/validation.test.ts +++ b/packages/firestore/test/integration/api/validation.test.ts @@ -809,6 +809,13 @@ apiDescribe('Validation:', persistence => { } ); + validationIt(persistence, 'with more than one != query fail', db => { + const coll = collection(db, 'test'); + expect(() => + query(coll, where('x', '!=', 32), where('x', '!=', 33)) + ).to.throw("Invalid query. You cannot use more than one '!=' filter."); + }); + validationIt(persistence, 'with != and not-in filters fail', db => { expect(() => query( @@ -832,16 +839,15 @@ apiDescribe('Validation:', persistence => { }); validationIt(persistence, 'with multiple disjunctive filters fail', db => { - // Mila: is this multiple inequality? - // expect(() => - // query( - // collection(db, 'test'), - // where('foo', 'not-in', [1, 2]), - // where('foo', 'not-in', [2, 3]) - // ) - // ).to.throw( - // "Invalid query. You cannot use more than one 'not-in' filter." - // ); + expect(() => + query( + collection(db, 'test'), + where('foo', 'not-in', [1, 2]), + where('foo', 'not-in', [2, 3]) + ) + ).to.throw( + "Invalid query. You cannot use more than one 'not-in' filter." + ); expect(() => query( @@ -1153,20 +1159,6 @@ apiDescribe('Validation:', persistence => { ).not.to.throw(); }); - validationIt(persistence, 'can have more than one !=', db => { - const coll = collection(db, 'test'); - expect(() => - query(coll, where('x', '!=', 32), where('y', '!=', 42)) - ).not.to.throw(); - }); - - validationIt(persistence, 'can have more than one not-in', db => { - const coll = collection(db, 'test'); - expect(() => - query(coll, where('x', 'not-in', [32]), where('y', 'not-in', [42])) - ).not.to.throw(); - }); - validationIt( persistence, 'can have != and inequality queries on different fields', @@ -1267,7 +1259,7 @@ apiDescribe('Validation:', persistence => { and(where('e', '==', 'f'), where('g', '!=', 'h')) ), or( - and(where('i', '==', 'j'), where('k', '!=', 'l')), + and(where('i', '==', 'j'), where('k', '>', 'l')), and(where('m', '==', 'n'), where('o', '<', 'p')) ) ) From 8d15a7f9c9899de32a429825078ece2c7e0349c1 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 12 Jul 2023 15:10:30 -0400 Subject: [PATCH 03/30] add order by rules --- packages/firestore/src/core/query.ts | 138 +++++------ packages/firestore/src/lite-api/query.ts | 63 +---- .../src/model/target_index_matcher.ts | 16 +- .../test/integration/api/database.test.ts | 6 +- .../test/integration/api/fields.test.ts | 120 +++++---- .../test/integration/api/query.test.ts | 91 ++++--- .../test/integration/api/validation.test.ts | 227 ++++++++++-------- .../firestore/test/unit/core/query.test.ts | 4 +- 8 files changed, 330 insertions(+), 335 deletions(-) diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index 1c67af211f4..3e489be0ea3 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -165,24 +165,6 @@ export function queryMatchesAllDocuments(query: Query): boolean { ); } -export function getFirstOrderByField(query: Query): FieldPath | null { - return query.explicitOrderBy.length > 0 - ? query.explicitOrderBy[0].field - : null; -} - -//Mila -export function getInequalityFilterField(query: Query): FieldPath | null { - for (const filter of query.filters) { - const result = filter.getFirstInequalityField(); - if (result !== null) { - return result; - } - } - - return null; -} - export function getInequalityFilterFields(query: Query): FieldPath[] | null { const result: FieldPath[] = []; for (const filter of query.filters) { @@ -191,10 +173,46 @@ export function getInequalityFilterFields(query: Query): FieldPath[] | null { result.push(field); } } - + return result.length === 0 ? null : result; } +function FiledPathLexicographicComparator( + fieldPathA: FieldPath, + fieldPathB: FieldPath +) { + // Document key field should be ordered to the last. + if (fieldPathA.isKeyField() && fieldPathB.isKeyField()) { + return 0; + } else if (fieldPathA.isKeyField() || fieldPathB.isKeyField()) { + return fieldPathA.isKeyField() ? 1 : -1; + } + + const segmentsA = fieldPathA.toArray(); + const segmentsB = fieldPathB.toArray(); + const minLength = Math.min(segmentsA.length, segmentsB.length); + + for (let i = 0; i < minLength; i++) { + const segmentA = segmentsA[i]; + const segmentB = segmentsB[i]; + + if (segmentA < segmentB) { + return -1; + } else if (segmentA > segmentB) { + return 1; + } + } + + // If the common segments are the same, compare the lengths + if (segmentsA.length < segmentsB.length) { + return -1; + } else if (segmentsA.length > segmentsB.length) { + return 1; + } + + return 0; +} + /** * Creates a new Query for a collection group query that matches all documents * within the provided collection group. @@ -233,48 +251,44 @@ export function queryOrderBy(query: Query): OrderBy[] { if (queryImpl.memoizedOrderBy === null) { queryImpl.memoizedOrderBy = []; - // //Note: Mila - const inequalityField = getInequalityFilterField(queryImpl); - const firstOrderByField = getFirstOrderByField(queryImpl); - if (inequalityField !== null && firstOrderByField === null) { - // In order to implicitly add key ordering, we must also add the - // inequality filter field for it to be a valid query. - // Note that the default inequality field and key ordering is ascending. - if (!inequalityField.isKeyField()) { - queryImpl.memoizedOrderBy.push(new OrderBy(inequalityField)); - } - queryImpl.memoizedOrderBy.push( - new OrderBy(FieldPath.keyField(), Direction.ASCENDING) - ); - } else { - //Note: Mila - // debugAssert( - // inequalityField === null || - // (firstOrderByField !== null && - // inequalityField.isEqual(firstOrderByField)), - // 'First orderBy should match inequality field.' - // ); - let foundKeyOrdering = false; - for (const orderBy of queryImpl.explicitOrderBy) { - queryImpl.memoizedOrderBy.push(orderBy); - if (orderBy.field.isKeyField()) { - foundKeyOrdering = true; + const fieldsIncluded = new Set(); + + // Add the explicit order by. + for (const orderBy of queryImpl.explicitOrderBy) { + queryImpl.memoizedOrderBy.push(orderBy); + fieldsIncluded.add(orderBy.field.canonicalString()); + } + + // The order of the implicit key ordering always matches the last + // explicit order by. + const lastDirection = + queryImpl.explicitOrderBy.length > 0 + ? queryImpl.explicitOrderBy[queryImpl.explicitOrderBy.length - 1].dir + : Direction.ASCENDING; + + const inequalityFields = getInequalityFilterFields(queryImpl); + + // Any inequality field not explicitly ordered should be implicitly ordered in a lexicographical order. + if (inequalityFields !== null) { + inequalityFields.sort(FiledPathLexicographicComparator); + for (const inequalityField of inequalityFields) { + if (!fieldsIncluded.has(inequalityField.canonicalString())) { + queryImpl.memoizedOrderBy.push( + new OrderBy(inequalityField, lastDirection) + ); + fieldsIncluded.add(inequalityField.canonicalString()); } } - if (!foundKeyOrdering) { - // The order of the implicit key ordering always matches the last - // explicit order by - const lastDirection = - queryImpl.explicitOrderBy.length > 0 - ? queryImpl.explicitOrderBy[queryImpl.explicitOrderBy.length - 1] - .dir - : Direction.ASCENDING; - queryImpl.memoizedOrderBy.push( - new OrderBy(FieldPath.keyField(), lastDirection) - ); - } + } + + // Add the document key to the last if it is not included. + if (!fieldsIncluded.has(FieldPath.keyField().canonicalString())) { + queryImpl.memoizedOrderBy.push( + new OrderBy(FieldPath.keyField(), lastDirection) + ); } } + return queryImpl.memoizedOrderBy; } @@ -329,16 +343,6 @@ export function queryToTarget(query: Query): Target { } export function queryWithAddedFilter(query: Query, filter: Filter): Query { - // Note:Mila - // const newInequalityField = filter.getFirstInequalityField(); - // const queryInequalityField = getInequalityFilterField(query); - // debugAssert( - // queryInequalityField == null || - // newInequalityField == null || - // newInequalityField.isEqual(queryInequalityField), - // 'Query must only have one inequality field.' - // ); - debugAssert( !isDocumentQuery(query), 'No filtering allowed for document query' diff --git a/packages/firestore/src/lite-api/query.ts b/packages/firestore/src/lite-api/query.ts index b7ed4f386d5..56414bf1b26 100644 --- a/packages/firestore/src/lite-api/query.ts +++ b/packages/firestore/src/lite-api/query.ts @@ -28,8 +28,6 @@ import { } from '../core/filter'; import { Direction, OrderBy } from '../core/order_by'; import { - getFirstOrderByField, - getInequalityFilterField, isCollectionGroupQuery, LimitType, Query as InternalQuery, @@ -868,8 +866,6 @@ export function newQueryOrderBy( ); } const orderBy = new OrderBy(fieldPath, direction); - // Mila - // validateNewOrderBy(query, orderBy); return orderBy; } @@ -1081,7 +1077,7 @@ function validateDisjunctiveFilterElements( function conflictingOps(op: Operator): Operator[] { switch (op) { case Operator.NOT_EQUAL: - return [ Operator.NOT_EQUAL, Operator.NOT_IN]; + return [Operator.NOT_EQUAL, Operator.NOT_IN]; case Operator.ARRAY_CONTAINS_ANY: case Operator.IN: return [Operator.NOT_IN]; @@ -1101,35 +1097,6 @@ function validateNewFieldFilter( query: InternalQuery, fieldFilter: FieldFilter ): void { - // Note: Mila - // if (fieldFilter.isInequality()) { - // const existingInequality = getInequalityFilterField(query); - // const newInequality = fieldFilter.field; - - // if ( - // existingInequality !== null && - // !existingInequality.isEqual(newInequality) - // ) { - // throw new FirestoreError( - // Code.INVALID_ARGUMENT, - // 'Invalid query. All where filters with an inequality' + - // ' (<, <=, !=, not-in, >, or >=) must be on the same field. But you have' + - // ` inequality filters on '${existingInequality.toString()}'` + - // ` and '${newInequality.toString()}'` - // ); - // } - - // const firstOrderByField = getFirstOrderByField(query); - // if (firstOrderByField !== null) { - // validateOrderByAndInequalityMatch( - // query, - // newInequality, - // firstOrderByField - // ); - // } - // } - - // Note:Mila const conflictingOp = findOpInsideFilters( query.filters, conflictingOps(fieldFilter.op) @@ -1177,34 +1144,6 @@ function findOpInsideFilters( return null; } -// function validateNewOrderBy(query: InternalQuery, orderBy: OrderBy): void { -// if (getFirstOrderByField(query) === null) { -// // This is the first order by. It must match any inequality. -// const inequalityField = getInequalityFilterField(query); -// if (inequalityField !== null) { -// validateOrderByAndInequalityMatch(query, inequalityField, orderBy.field); -// } -// } -// } - -// function validateOrderByAndInequalityMatch( -// baseQuery: InternalQuery, -// inequality: InternalFieldPath, -// orderBy: InternalFieldPath -// ): void { -// if (!orderBy.isEqual(inequality)) { -// // Note:Mila -// throw new FirestoreError( -// Code.INVALID_ARGUMENT, -// `Invalid query. You have a where filter with an inequality ` + -// `(<, <=, !=, not-in, >, or >=) on field '${inequality.toString()}' ` + -// `and so you must also use '${inequality.toString()}' ` + -// `as your first argument to orderBy(), but your first orderBy() ` + -// `is on field '${orderBy.toString()}' instead.` -// ); -// } -// } - export function validateQueryFilterConstraint( functionName: string, queryConstraint: AppliableConstraint diff --git a/packages/firestore/src/model/target_index_matcher.ts b/packages/firestore/src/model/target_index_matcher.ts index f8763d3b2b9..4dbeb734405 100644 --- a/packages/firestore/src/model/target_index_matcher.ts +++ b/packages/firestore/src/model/target_index_matcher.ts @@ -53,7 +53,7 @@ export class TargetIndexMatcher { // The collection ID (or collection group) of the query target. private readonly collectionId: string; // The inequality filters of the target (if it exists). - private readonly inequalityFilters = new Map(); + private readonly inequalityFilters = new Map(); // The list of equality filters of the target. private readonly equalityFilters: FieldFilter[]; // The list of orderBys of the target. @@ -66,11 +66,13 @@ export class TargetIndexMatcher { : target.path.lastSegment(); this.orderBys = target.orderBy; this.equalityFilters = []; - // Note:Mila for (const filter of target.filters) { const fieldFilter = filter as FieldFilter; if (fieldFilter.isInequality()) { - this.inequalityFilters.set(fieldFilter.field.canonicalString(), fieldFilter); + this.inequalityFilters.set( + fieldFilter.field.canonicalString(), + fieldFilter + ); } else { this.equalityFilters.push(fieldFilter); } @@ -142,8 +144,8 @@ export class TargetIndexMatcher { } if (this.inequalityFilters.size > 0) { - if (this.inequalityFilters.size > 1) { - // Only single inequality is supported now. + if (this.inequalityFilters.size > 1) { + // Only single inequality is supported for now. return false; } @@ -151,9 +153,7 @@ export class TargetIndexMatcher { // 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 ( - !equalitySegments.has(inequalityFilter.field.canonicalString()) - ) { + if (!equalitySegments.has(inequalityFilter.field.canonicalString())) { const segment = segments[segmentIndex]; if ( diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index dd08f9561d7..44a5a2046f5 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -670,7 +670,11 @@ apiDescribe('Database', persistence => { it('multiple inequality with key fields works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => - query(coll, where(documentId(), ">=", ref('collection/id')), where('x', '>=', 32)) + query( + coll, + where(documentId(), '>=', ref('collection/id')), + where('x', '>=', 32) + ) ).not.to.throw(); }); }); diff --git a/packages/firestore/test/integration/api/fields.test.ts b/packages/firestore/test/integration/api/fields.test.ts index 486a6c616a6..f51e4535e27 100644 --- a/packages/firestore/test/integration/api/fields.test.ts +++ b/packages/firestore/test/integration/api/fields.test.ts @@ -320,8 +320,6 @@ apiDescribe('Fields with special characters', persistence => { }); }); - - it('can be used in a query orderBy.', () => { const testDocs = { '1': testData(300), @@ -342,60 +340,86 @@ apiDescribe('Fields with special characters', persistence => { }); }); - // eslint-disable-next-line no-restricted-properties -(USE_EMULATOR ? apiDescribe : apiDescribe.skip)('Multiple Inequality', persistence => { - const testData = (n?: number): AnyTestData => { - n = n || 1; - return { - name: 'room ' + n, - metadata: { - createdAt: n, - }, - field: 'field ' + n, - 'field.dot': n, - 'field\\slash': n - }; - }; - it('can be used with query.where().', () => { - const testDocs = { - '1': testData(100), - '2': testData(200), - '3': testData(500), - '4': testData(300), +// eslint-disable-next-line no-restricted-properties +(USE_EMULATOR ? apiDescribe : apiDescribe.skip)( + 'Fields in Multiple Inequality', + persistence => { + const testData = (n?: number): AnyTestData => { + n = n || 1; + return { + name: 'room ' + n, + metadata: { + createdAt: n + }, + field: 'field ' + n, + 'field.dot': n, + 'field\\slash': n + }; }; - return withTestCollection(persistence, testDocs, coll => { - return getDocs(query(coll, where('metadata.createdAt', '<=', 500), where('metadata.createdAt', '>', 100), where('name', '!=', 'room 200'), orderBy('name'))).then( - results => { + + it('can be used with query.where().', () => { + const testDocs = { + '1': testData(100), + '2': testData(200), + '3': testData(500), + '4': testData(300) + }; + return withTestCollection(persistence, testDocs, coll => { + return getDocs( + query( + coll, + where('metadata.createdAt', '<=', 500), + where('metadata.createdAt', '>', 100), + where('name', '!=', 'room 200'), + orderBy('name') + ) + ).then(results => { // inequality adds implicit sort on field expect(toDataArray(results)).to.deep.equal([ testData(300), - testData(500), + testData(500) ]); - } - ); - }); - }); - - it('can be used in multiple inequality query filters.', () => { - const testDocs = { - '1': testData(400), - '2': testData(100), - '3': testData(200), - '4': testData(300), - }; - return withTestCollection(persistence, testDocs, coll => { - return getDocs(query(coll, where('field', '>=', 'field 200'), where(new FieldPath('field.dot'), '!=', 300))) - .then(results => { - expect(toDataArray(results)).to.deep.equal([testData(200), testData(400)]); - }) - .then(() => getDocs(query(coll, where('field', '<=', 'field 200'), where('field\\slash', '>=', 200)))) - .then(results => { - expect(toDataArray(results)).to.deep.equal([testData(200)]); }); + }); }); - }); - }) + it('can be used in multiple inequality query filters.', () => { + const testDocs = { + '1': testData(400), + '2': testData(100), + '3': testData(200), + '4': testData(300) + }; + return withTestCollection(persistence, testDocs, coll => { + return getDocs( + query( + coll, + where('field', '>=', 'field 200'), + where(new FieldPath('field.dot'), '!=', 300) + ) + ) + .then(results => { + expect(toDataArray(results)).to.deep.equal([ + testData(200), + testData(400) + ]); + }) + .then(() => + getDocs( + query( + coll, + where('field', '<=', 'field 200'), + where('field\\slash', '>=', 200) + ) + ) + ) + .then(results => { + expect(toDataArray(results)).to.deep.equal([testData(200)]); + }); + }); + }); + } +); apiDescribe('Timestamp Fields in snapshots', persistence => { // Figure out how to pass in the Timestamp type diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 40a9729191c..a320042474b 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1335,53 +1335,52 @@ apiDescribe('Queries', persistence => { }); // eslint-disable-next-line no-restricted-properties - ( USE_EMULATOR ? describe : describe.skip)( - 'OR Queries', - () => { - it('key order is descending for descending multiple inequality', () => { - const testDocs = { - a: { - foo: 42, - bar: 'ab' - }, - b: { - foo: 42.0, - bar: 'aa' - }, - c: { - foo: 42, - bar: 'ba' - }, - d: { - foo: 21, - bar: 'b' - }, - e: { - foo: 21, - bar: 'a' - }, - f: { - foo: 66, - bar: 'ac' - }, - g: { - foo: 66, - bar: 'c' - } - }; - return withTestCollection(persistence, testDocs, coll => { - return getDocs( - query(coll, where('foo', '>', 21.0),where('bar', 'not-in', ['a','ac','ba']), orderBy('foo', 'desc') ) - ).then(docs => { - expect(docs.docs.map(d => d.id)).to.deep.equal([ - 'g', 'b', 'a' - ]); - }); + (USE_EMULATOR ? describe : describe.skip)('Multiple Inequality', () => { + it('key order is descending for descending multiple inequality', () => { + const testDocs = { + a: { + foo: 42, + bar: 'ab' + }, + b: { + foo: 42.0, + bar: 'aa' + }, + c: { + foo: 42, + bar: 'ba' + }, + d: { + foo: 21, + bar: 'b' + }, + e: { + foo: 21, + bar: 'a' + }, + f: { + foo: 66, + bar: 'ac' + }, + g: { + foo: 66, + bar: 'c' + } + }; + return withTestCollection(persistence, testDocs, coll => { + return getDocs( + query( + coll, + where('foo', '>', 21.0), + where('bar', 'not-in', ['a', 'ac', 'ba']), + orderBy('foo', 'desc') + ) + ).then(docs => { + expect(docs.docs.map(d => d.id)).to.deep.equal(['g', 'a', 'b']); }); }); - - } - ); + }); + }); // OR Query tests only run when the SDK's local cache is configured to use // LRU garbage collection (rather than eager garbage collection) because @@ -1748,7 +1747,7 @@ apiDescribe('Queries', persistence => { await checkOnlineAndOfflineResultsMatch( query(coll, or(where('a', '>', 2), where('b', '<', 1))), 'doc1', - 'doc3', + 'doc3' ); // Test with limits (implicit order by ASC): (a==1) || (b > 0) LIMIT 2 diff --git a/packages/firestore/test/integration/api/validation.test.ts b/packages/firestore/test/integration/api/validation.test.ts index e130695b40f..c65e54da753 100644 --- a/packages/firestore/test/integration/api/validation.test.ts +++ b/packages/firestore/test/integration/api/validation.test.ts @@ -1187,134 +1187,159 @@ apiDescribe('Validation:', persistence => { db => { const coll = collection(db, 'test'); // single inequality - expect(() => query(coll, where('x', '>', 32), orderBy('y'))).not.to.throw(); - expect(() => query(coll, orderBy('y'), where('x', '>', 32))).not.to.throw(); + expect(() => + query(coll, where('x', '>', 32), orderBy('y')) + ).not.to.throw(); + expect(() => + query(coll, orderBy('y'), where('x', '>', 32)) + ).not.to.throw(); expect(() => query(coll, where('x', '>', 32), orderBy('y'), orderBy('x')) ).not.to.throw(); expect(() => query(coll, orderBy('y'), orderBy('x'), where('x', '>', 32)) ).not.to.throw(); - expect(() => query(coll, where('x', '!=', 32), orderBy('y'))).not.to.throw(); + expect(() => + query(coll, where('x', '!=', 32), orderBy('y')) + ).not.to.throw(); // multiple inequality - expect(() => query(coll, where('x', '>', 32), where('y', '!=', 42), orderBy('z'))).not.to.throw(); - expect(() => query(coll, orderBy('y'), where('x', '>', 32), where('y', '<=', 42))).not.to.throw(); expect(() => - query(coll, where('x', '>', 32), where('y', '!=', 42), orderBy('y'), orderBy('x')) + query(coll, where('x', '>', 32), where('y', '!=', 42), orderBy('z')) ).not.to.throw(); expect(() => - query(coll, orderBy('y'), orderBy('z'), where('x', '>', 32), where('y', '!=', 42)) + query(coll, orderBy('y'), where('x', '>', 32), where('y', '<=', 42)) + ).not.to.throw(); + expect(() => + query( + coll, + where('x', '>', 32), + where('y', '!=', 42), + orderBy('y'), + orderBy('x') + ) + ).not.to.throw(); + expect(() => + query( + coll, + orderBy('y'), + orderBy('z'), + where('x', '>', 32), + where('y', '!=', 42) + ) ).not.to.throw(); } ); - validationIt(persistence, 'multiple inequalities inside a nested composite filter', db => { - // Multiple inequality on different fields inside a nested composite filter. - const coll = collection(db, 'test'); - expect(() => - query( - coll, - or( - and(where('a', '==', 'b'), where('c', '>', 'd')), - and(where('e', '<=', 'f'), where('g', '==', 'h')) - ) - ) - ).not.to.throw(); - - // Multiple inequality on different fields between a field filter and a composite filter. - expect(() => - query( - coll, - and( + validationIt( + persistence, + 'multiple inequalities inside a nested composite filter', + db => { + // Multiple inequality on different fields inside a nested composite filter. + const coll = collection(db, 'test'); + expect(() => + query( + coll, or( - and(where('a', '==', 'b'), where('c', '>=', 'd')), - and(where('e', '==', 'f'), where('g', '!=', 'h')) - ), - where('r', '<', 's') + and(where('a', '==', 'b'), where('c', '>', 'd')), + and(where('e', '<=', 'f'), where('g', '==', 'h')) + ) ) - ) - ).not.to.throw(); + ).not.to.throw(); - // OrderBy and multiple inequality on different fields. - expect(() => - query( - coll, - or( - and(where('a', '==', 'b'), where('c', '>', 'd')), - and(where('e', '==', 'f'), where('g', '!=', 'h')) - ), - orderBy('r'), - orderBy('a') - ) - ).not.to.throw(); + // Multiple inequality on different fields between a field filter and a composite filter. + expect(() => + query( + coll, + and( + or( + and(where('a', '==', 'b'), where('c', '>=', 'd')), + and(where('e', '==', 'f'), where('g', '!=', 'h')) + ), + where('r', '<', 's') + ) + ) + ).not.to.throw(); - // Multiple inequality inside two composite filters. - expect(() => - query( - coll, - and( + // OrderBy and multiple inequality on different fields. + expect(() => + query( + coll, or( - and(where('a', '==', 'b'), where('c', '>=', 'd')), + and(where('a', '==', 'b'), where('c', '>', 'd')), and(where('e', '==', 'f'), where('g', '!=', 'h')) ), - or( - and(where('i', '==', 'j'), where('k', '>', 'l')), - and(where('m', '==', 'n'), where('o', '<', 'p')) + orderBy('r'), + orderBy('a') + ) + ).not.to.throw(); + + // Multiple inequality inside two composite filters. + expect(() => + query( + coll, + and( + or( + and(where('a', '==', 'b'), where('c', '>=', 'd')), + and(where('e', '==', 'f'), where('g', '!=', 'h')) + ), + or( + and(where('i', '==', 'j'), where('k', '>', 'l')), + and(where('m', '==', 'n'), where('o', '<', 'p')) + ) ) ) - ) - ).not.to.throw(); + ).not.to.throw(); - // Multiple top level composite filters - expect(() => - // @ts-ignore - query(coll, and(where('a', '==', 'b')), or(where('b', '==', 'a'))) - ).to.throw( - 'InvalidQuery. When using composite filters, you cannot use ' + - 'more than one filter at the top level. Consider nesting the multiple ' + - 'filters within an `and(...)` statement. For example: ' + - 'change `query(query, where(...), or(...))` to ' + - '`query(query, and(where(...), or(...)))`.' - ); + // Multiple top level composite filters + expect(() => + // @ts-ignore + query(coll, and(where('a', '==', 'b')), or(where('b', '==', 'a'))) + ).to.throw( + 'InvalidQuery. When using composite filters, you cannot use ' + + 'more than one filter at the top level. Consider nesting the multiple ' + + 'filters within an `and(...)` statement. For example: ' + + 'change `query(query, where(...), or(...))` to ' + + '`query(query, and(where(...), or(...)))`.' + ); - // Once top level composite filter and one top level field filter - expect(() => - // @ts-ignore - query(coll, or(where('a', '==', 'b')), where('b', '==', 'a')) - ).to.throw( - 'InvalidQuery. When using composite filters, you cannot use ' + - 'more than one filter at the top level. Consider nesting the multiple ' + - 'filters within an `and(...)` statement. For example: ' + - 'change `query(query, where(...), or(...))` to ' + - '`query(query, and(where(...), or(...)))`.' - ); + // Once top level composite filter and one top level field filter + expect(() => + // @ts-ignore + query(coll, or(where('a', '==', 'b')), where('b', '==', 'a')) + ).to.throw( + 'InvalidQuery. When using composite filters, you cannot use ' + + 'more than one filter at the top level. Consider nesting the multiple ' + + 'filters within an `and(...)` statement. For example: ' + + 'change `query(query, where(...), or(...))` to ' + + '`query(query, and(where(...), or(...)))`.' + ); - // Multiple top level composite filters - expect(() => - // @ts-ignore - query(coll, and(where('a', '==', 'b')), or(where('b', '==', 'a'))) - ).to.throw( - 'InvalidQuery. When using composite filters, you cannot use ' + - 'more than one filter at the top level. Consider nesting the multiple ' + - 'filters within an `and(...)` statement. For example: ' + - 'change `query(query, where(...), or(...))` to ' + - '`query(query, and(where(...), or(...)))`.' - ); - - // Once top level composite filter and one top level field filter - expect(() => - // @ts-ignore - query(coll, or(where('a', '==', 'b')), where('b', '==', 'a')) - ).to.throw( - 'InvalidQuery. When using composite filters, you cannot use ' + - 'more than one filter at the top level. Consider nesting the multiple ' + - 'filters within an `and(...)` statement. For example: ' + - 'change `query(query, where(...), or(...))` to ' + - '`query(query, and(where(...), or(...)))`.' - ); - }); - + // Multiple top level composite filters + expect(() => + // @ts-ignore + query(coll, and(where('a', '==', 'b')), or(where('b', '==', 'a'))) + ).to.throw( + 'InvalidQuery. When using composite filters, you cannot use ' + + 'more than one filter at the top level. Consider nesting the multiple ' + + 'filters within an `and(...)` statement. For example: ' + + 'change `query(query, where(...), or(...))` to ' + + '`query(query, and(where(...), or(...)))`.' + ); + + // Once top level composite filter and one top level field filter + expect(() => + // @ts-ignore + query(coll, or(where('a', '==', 'b')), where('b', '==', 'a')) + ).to.throw( + 'InvalidQuery. When using composite filters, you cannot use ' + + 'more than one filter at the top level. Consider nesting the multiple ' + + 'filters within an `and(...)` statement. For example: ' + + 'change `query(query, where(...), or(...))` to ' + + '`query(query, and(where(...), or(...)))`.' + ); + } + ); }); }); diff --git a/packages/firestore/test/unit/core/query.test.ts b/packages/firestore/test/unit/core/query.test.ts index 064f6214a77..49c17d70e4d 100644 --- a/packages/firestore/test/unit/core/query.test.ts +++ b/packages/firestore/test/unit/core/query.test.ts @@ -820,7 +820,7 @@ describe('Query', () => { orFilter(filter('a', '>', 2), filter('b', '==', 1)) ); assertQueryMatches(query2, [doc2, doc3, doc5], [doc1, doc4]); - + // (a==1 && b==0) || (a==3 && b==2) const query3 = query( 'collection', @@ -894,7 +894,7 @@ describe('Query', () => { orFilter(filter('a', '>=', 3), filter('b', '<', 3)) ) ); - assertQueryMatches(query5, [doc1, doc2, doc3 ], [ doc4, doc5]); + assertQueryMatches(query5, [doc1, doc2, doc3], [doc4, doc5]); }); function assertQueryMatches( From 7d17790ce97e1dcef9725c47bb51a63435812de4 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 12 Jul 2023 17:54:07 -0400 Subject: [PATCH 04/30] format --- .../test/integration/api/database.test.ts | 22 +- .../test/integration/api/fields.test.ts | 81 ----- .../test/integration/api/query.test.ts | 332 ++++++++++++++++-- 3 files changed, 316 insertions(+), 119 deletions(-) diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 44a5a2046f5..1d72596fdf9 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -667,7 +667,7 @@ apiDescribe('Database', persistence => { }); }); - it('multiple inequality with key fields works', () => { + it('inequality with key fields works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => query( @@ -788,6 +788,26 @@ apiDescribe('Database', persistence => { ).not.to.throw(); }); }); + it('multiple inequality different from orderBy works.', () => { + return withTestCollection(persistence, {}, async coll => { + expect(() => + query( + coll, + where('x', '>', 32), + where('y', '!=', 'cat'), + orderBy('z') + ) + ).not.to.throw(); + expect(() => + query( + coll, + orderBy('z'), + where('x', '>', 32), + where('y', '!=', 'cat') + ) + ).not.to.throw(); + }); + }); it('array-contains different than orderBy works', () => { return withTestCollection(persistence, {}, async coll => { diff --git a/packages/firestore/test/integration/api/fields.test.ts b/packages/firestore/test/integration/api/fields.test.ts index f51e4535e27..306a7f51ddd 100644 --- a/packages/firestore/test/integration/api/fields.test.ts +++ b/packages/firestore/test/integration/api/fields.test.ts @@ -340,87 +340,6 @@ apiDescribe('Fields with special characters', persistence => { }); }); -// eslint-disable-next-line no-restricted-properties -(USE_EMULATOR ? apiDescribe : apiDescribe.skip)( - 'Fields in Multiple Inequality', - persistence => { - const testData = (n?: number): AnyTestData => { - n = n || 1; - return { - name: 'room ' + n, - metadata: { - createdAt: n - }, - field: 'field ' + n, - 'field.dot': n, - 'field\\slash': n - }; - }; - - it('can be used with query.where().', () => { - const testDocs = { - '1': testData(100), - '2': testData(200), - '3': testData(500), - '4': testData(300) - }; - return withTestCollection(persistence, testDocs, coll => { - return getDocs( - query( - coll, - where('metadata.createdAt', '<=', 500), - where('metadata.createdAt', '>', 100), - where('name', '!=', 'room 200'), - orderBy('name') - ) - ).then(results => { - // inequality adds implicit sort on field - expect(toDataArray(results)).to.deep.equal([ - testData(300), - testData(500) - ]); - }); - }); - }); - - it('can be used in multiple inequality query filters.', () => { - const testDocs = { - '1': testData(400), - '2': testData(100), - '3': testData(200), - '4': testData(300) - }; - return withTestCollection(persistence, testDocs, coll => { - return getDocs( - query( - coll, - where('field', '>=', 'field 200'), - where(new FieldPath('field.dot'), '!=', 300) - ) - ) - .then(results => { - expect(toDataArray(results)).to.deep.equal([ - testData(200), - testData(400) - ]); - }) - .then(() => - getDocs( - query( - coll, - where('field', '<=', 'field 200'), - where('field\\slash', '>=', 200) - ) - ) - ) - .then(results => { - expect(toDataArray(results)).to.deep.equal([testData(200)]); - }); - }); - }); - } -); - apiDescribe('Timestamp Fields in snapshots', persistence => { // Figure out how to pass in the Timestamp type // eslint-disable-next-line @typescript-eslint/no-explicit-any diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index a320042474b..862b5de9faf 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -36,6 +36,7 @@ import { enableNetwork, endAt, endBefore, + FieldPath, GeoPoint, getDocs, getDocsFromCache, @@ -1336,48 +1337,305 @@ apiDescribe('Queries', persistence => { // eslint-disable-next-line no-restricted-properties (USE_EMULATOR ? describe : describe.skip)('Multiple Inequality', () => { - it('key order is descending for descending multiple inequality', () => { + it('can use multiple inequality filters', async () => { const testDocs = { - a: { - foo: 42, - bar: 'ab' - }, - b: { - foo: 42.0, - bar: 'aa' - }, - c: { - foo: 42, - bar: 'ba' - }, - d: { - foo: 21, - bar: 'b' - }, - e: { - foo: 21, - bar: 'a' - }, - f: { - foo: 66, - bar: 'ac' - }, - g: { - foo: 66, - bar: 'c' - } + doc1: { key: 'a', sort: 0, v: 0 }, + doc2: { key: 'b', sort: 3, v: 1 }, + doc3: { key: 'c', sort: 1, v: 2 }, + doc4: { key: 'd', sort: 2, v: 3 } }; - return withTestCollection(persistence, testDocs, coll => { - return getDocs( + await withTestCollection(persistence, testDocs, async coll => { + // multiple inequality fields + const snapshot1 = await getDocs( + query(coll, where('key', '!=', 'a'), where('sort', '<=', 2)) + ); + expect(toDataArray(snapshot1)).to.deep.equal([ + { key: 'c', sort: 1, v: 2 }, + { key: 'd', sort: 2, v: 3 } + ]); + + const snapshot2 = await getDocs( query( coll, - where('foo', '>', 21.0), - where('bar', 'not-in', ['a', 'ac', 'ba']), - orderBy('foo', 'desc') + where('key', '!=', 'a'), + where('sort', '<=', 2), + where('v', '>', 2) ) - ).then(docs => { - expect(docs.docs.map(d => d.id)).to.deep.equal(['g', 'a', 'b']); - }); + ); + expect(toDataArray(snapshot2)).to.deep.equal([ + { key: 'd', sort: 2, v: 3 } + ]); + + // duplicate inequality fields + const snapshot3 = await getDocs( + query( + coll, + where('key', '!=', 'a'), + where('sort', '<=', 2), + where('sort', '>', 1) + ) + ); + expect(toDataArray(snapshot3)).to.deep.equal([ + { key: 'd', sort: 2, v: 3 } + ]); + + // With NOT-IN operator + const snapshot4 = await getDocs( + query( + coll, + where('key', '>=', 'a'), + where('sort', '<=', 2), + where('v', 'not-in', [2, 4, 5]) + ) + ); + expect(toDataArray(snapshot4)).to.deep.equal([ + { key: 'a', sort: 0, v: 0 }, + { key: 'd', sort: 2, v: 3 } + ]); + }); + }); + + it('can use multiple inequality with nested field', () => { + const testData = (n?: number): any => { + n = n || 1; + return { + name: 'room ' + n, + metadata: { + createdAt: n + }, + field: 'field ' + n, + 'field.dot': n, + 'field\\slash': n + }; + }; + + const testDocs = { + '1': testData(400), + '2': testData(200), + '3': testData(100), + '4': testData(300) + }; + + return withTestCollection(persistence, testDocs, async coll => { + const snapshot1 = await getDocs( + query( + coll, + where('metadata.createdAt', '<=', 500), + where('metadata.createdAt', '>', 100), + where('name', '!=', 'room 200'), + orderBy('name') + ) + ); + expect(toDataArray(snapshot1)).to.deep.equal([ + testData(300), + testData(400) + ]); + + const snapshot2 = await getDocs( + query( + coll, + where('field', '>=', 'field 100'), + where(new FieldPath('field.dot'), '!=', 300), + where('field\\slash', '<', 400), + orderBy('name','desc') + ) + ); + expect(toDataArray(snapshot2)).to.deep.equal([ + testData(200), + testData(100) + ]); + }); + }); + + + it.only('can use with nested composite filters', async () => { + const testDocs = { + doc1: { key: 'a', sort: 0, v: 5 }, + doc2: { key: 'aa', sort: 4, v: 4 }, + doc3: { key: 'b', sort: 3, v: 3 }, + doc4: { key: 'b', sort: 2, v: 2 }, + doc5: { key: 'b', sort: 2, v: 1 }, + doc6: { key: 'b', sort: 0, v: 0 } + }; + await withTestCollection(persistence, testDocs, async coll => { + const snapshot1 = await getDocs( + query(coll, + or( + and(where('key', '==', 'b'), where('sort', '<=',2)), + and(where('key', '!=', 'b'), where('v',">",4)) + )) + ); + // Implicitly ordered by: 'sort' asc, 'v' asc, __name__ asc + expect(toDataArray(snapshot1)).to.deep.equal([ + { key: 'a', sort: 0, v: 5 }, + { key: 'b', sort: 0, v: 0 }, + { key: 'b', sort: 2, v: 1 }, + { key: 'b', sort: 2, v: 2 }, + ]); + + // const snapshot2 = await getDocs( + // query( + // coll, + // or( + // and(where('key', '==', 'b'), where('sort', '<=',2)), + // and(where('v', 'in', [2,3,4]), where('sort',">",3)) + // ), + // orderBy('sort', 'desc'), + // orderBy('key') + // ) + // ); + // expect(toDataArray(snapshot2)).to.deep.equal([ + // { key: 'aa', sort: 4, v: 4 }, + // { key: 'b', sort: 2, v: 2 }, + // { key: 'b', sort: 2, v: 1 }, + // { key: 'b', sort: 0, v: 0 }, + // ]); + + + // const snapshot3 = await getDocs( + // query( + // coll, + // and( + // or( + // and(where('a', '==', 'b'), where('c', '>=', 'd')), + // and(where('e', '==', 'f'), where('g', '!=', 'h')) + // ), + // or( + // and(where('i', '==', 'j'), where('k', '>', 'l')), + // and(where('m', '==', 'n'), where('o', '<', 'p')) + // ) + // ) + // )); + // expect(toDataArray(snapshot3)).to.deep.equal([ + // { key: 'aa', sort: 4, v: 4 }, + // { key: 'b', sort: 2, v: 2 }, + // { key: 'b', sort: 2, v: 1 }, + // { key: 'b', sort: 3, v: 3 } + // ]); + }); + }); + + it('inequality fields will be implicitly ordered lexicographically', async () => { + const testDocs = { + doc1: { key: 'a', sort: 0, v: 5 }, + doc2: { key: 'aa', sort: 4, v: 4 }, + doc3: { key: 'b', sort: 3, v: 3 }, + doc4: { key: 'b', sort: 2, v: 2 }, + doc5: { key: 'b', sort: 2, v: 1 }, + doc6: { key: 'b', sort: 0, v: 0 } + }; + await withTestCollection(persistence, testDocs, async coll => { + // Implicitly ordered by: 'key' asc, 'sort' asc, __name__ asc + const snapshot1 = await getDocs( + query( + coll, + where('key', '!=', 'a'), + where('sort', '>', 1), + where('v', 'in', [1, 2, 3, 4]) + ) + ); + expect(toDataArray(snapshot1)).to.deep.equal([ + { key: 'aa', sort: 4, v: 4 }, + { key: 'b', sort: 2, v: 2 }, + { key: 'b', sort: 2, v: 1 }, + { key: 'b', sort: 3, v: 3 } + ]); + + // Implicitly ordered by: 'key' asc, 'sort' asc,__name__ asc + const snapshot2 = await getDocs( + query( + coll, + where('sort', '>', 1), + where('key', '!=', 'a'), + where('v', 'in', [1, 2, 3, 4]) + ) + ); + expect(toDataArray(snapshot2)).to.deep.equal([ + { key: 'aa', sort: 4, v: 4 }, + { key: 'b', sort: 2, v: 2 }, + { key: 'b', sort: 2, v: 1 }, + { key: 'b', sort: 3, v: 3 } + ]); + }); + }); + + it('can use multiple explicit order by field', async () => { + const testDocs = { + doc1: { key: 'a', sort: 5, v: 0 }, + doc2: { key: 'aa', sort: 4, v: 0 }, + doc3: { key: 'b', sort: 3, v: 1 }, + doc4: { key: 'b', sort: 2, v: 1 }, + doc5: { key: 'bb', sort: 1, v: 1 }, + doc6: { key: 'c', sort: 0, v: 2 } + }; + + await withTestCollection(persistence, testDocs, async coll => { + // Ordered by: 'v' asc, 'key' asc, 'sort' asc, __name__ asc + const snapshot1 = await getDocs( + query( + coll, + where('key', '>', 'a'), + where('sort', '>=', 1), + orderBy('v') + ) + ); + expect(toDataArray(snapshot1)).to.deep.equal([ + { key: 'aa', sort: 4, v: 0 }, + { key: 'b', sort: 2, v: 1 }, + { key: 'b', sort: 3, v: 1 }, + { key: 'bb', sort: 1, v: 1 } + ]); + + // Ordered by: 'v asc, 'sort' asc, 'key' asc, __name__ asc + const snapshot2 = await getDocs( + query( + coll, + where('key', '>', 'a'), + where('sort', '>=', 1), + orderBy('v'), + orderBy('sort') + ) + ); + expect(toDataArray(snapshot2)).to.deep.equal([ + { key: 'aa', sort: 4, v: 0 }, + { key: 'bb', sort: 1, v: 1 }, + { key: 'b', sort: 2, v: 1 }, + { key: 'b', sort: 3, v: 1 } + ]); + + // Implicit order by matches the direction of last explicit order by. + // Ordered by: 'v' desc, 'key' desc, 'sort' desc, __name__ desc + const snapshot3 = await getDocs( + query( + coll, + where('key', '>', 'a'), + where('sort', '>=', 1), + orderBy('v', 'desc') + ) + ); + expect(toDataArray(snapshot3)).to.deep.equal([ + { key: 'bb', sort: 1, v: 1 }, + { key: 'b', sort: 3, v: 1 }, + { key: 'b', sort: 2, v: 1 }, + { key: 'aa', sort: 4, v: 0 } + ]); + + // Ordered by: 'v desc, 'sort' asc, 'key' asc, __name__ asc + const snapshot4 = await getDocs( + query( + coll, + where('key', '>', 'a'), + where('sort', '>=', 1), + orderBy('v', 'desc'), + orderBy('sort') + ) + ); + expect(toDataArray(snapshot4)).to.deep.equal([ + { key: 'bb', sort: 1, v: 1 }, + { key: 'b', sort: 2, v: 1 }, + { key: 'b', sort: 3, v: 1 }, + { key: 'aa', sort: 4, v: 0 } + ]); }); }); }); From 5a29be477673b56dcc809f0dfeb1cfec8181f507 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 12 Jul 2023 20:48:08 -0400 Subject: [PATCH 05/30] add composite ineuquality field implicit orderby --- packages/firestore/src/core/filter.ts | 36 +++---- packages/firestore/src/core/query.ts | 12 +-- .../test/integration/api/query.test.ts | 93 +++++++++---------- 3 files changed, 65 insertions(+), 76 deletions(-) diff --git a/packages/firestore/src/core/filter.ts b/packages/firestore/src/core/filter.ts index 16bb627f2dc..918828f313a 100644 --- a/packages/firestore/src/core/filter.ts +++ b/packages/firestore/src/core/filter.ts @@ -57,7 +57,7 @@ export abstract class Filter { abstract getFilters(): Filter[]; - abstract getFirstInequalityField(): FieldPath | null; + abstract getInequalityFilters(): readonly FieldFilter[] | null; } export class FieldFilter extends Filter { @@ -195,9 +195,9 @@ export class FieldFilter extends Filter { return [this]; } - getFirstInequalityField(): FieldPath | null { + getInequalityFilters(): readonly FieldFilter[] | null { if (this.isInequality()) { - return this.field; + return [this]; } return null; } @@ -247,28 +247,18 @@ export class CompositeFilter extends Filter { return Object.assign([], this.filters); } - getFirstInequalityField(): FieldPath | null { - const found = this.findFirstMatchingFilter(filter => filter.isInequality()); + // Performs a depth-first search to find and return the inequality FieldFilters in the composite + // filter. Returns `null` if none of the FieldFilters satisfy the predicate. + getInequalityFilters(): readonly FieldFilter[] | null { + const result: FieldFilter[] = []; - if (found !== null) { - return found.field; - } - return null; - } - - // Performs a depth-first search to find and return the first FieldFilter in the composite filter - // that satisfies the predicate. Returns `null` if none of the FieldFilters satisfy the - // predicate. - private findFirstMatchingFilter( - predicate: (filter: FieldFilter) => boolean - ): FieldFilter | null { - for (const fieldFilter of this.getFlattenedFilters()) { - if (predicate(fieldFilter)) { - return fieldFilter; + this.getFlattenedFilters().forEach((fieldFilter: FieldFilter) => { + const subFilters = fieldFilter.getInequalityFilters(); + if (subFilters !== null) { + result.push(...subFilters); } - } - - return null; + }); + return result.length === 0 ? null : result; } } diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index 3e489be0ea3..e10b621cf6a 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -167,12 +167,13 @@ export function queryMatchesAllDocuments(query: Query): boolean { export function getInequalityFilterFields(query: Query): FieldPath[] | null { const result: FieldPath[] = []; - for (const filter of query.filters) { - const field = filter.getFirstInequalityField(); - if (field !== null) { - result.push(field); + + query.filters.forEach((filter: Filter) => { + const inequalityFilters = filter.getInequalityFilters(); + if (inequalityFilters !== null) { + result.push(...inequalityFilters.map(filter => filter.field)); } - } + }); return result.length === 0 ? null : result; } @@ -248,7 +249,6 @@ export function isCollectionGroupQuery(query: Query): boolean { */ export function queryOrderBy(query: Query): OrderBy[] { const queryImpl = debugCast(query, QueryImpl); - if (queryImpl.memoizedOrderBy === null) { queryImpl.memoizedOrderBy = []; const fieldsIncluded = new Set(); diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 862b5de9faf..709295a36ed 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1437,7 +1437,7 @@ apiDescribe('Queries', persistence => { where('field', '>=', 'field 100'), where(new FieldPath('field.dot'), '!=', 300), where('field\\slash', '<', 400), - orderBy('name','desc') + orderBy('name', 'desc') ) ); expect(toDataArray(snapshot2)).to.deep.equal([ @@ -1446,72 +1446,71 @@ apiDescribe('Queries', persistence => { ]); }); }); - it.only('can use with nested composite filters', async () => { const testDocs = { doc1: { key: 'a', sort: 0, v: 5 }, doc2: { key: 'aa', sort: 4, v: 4 }, - doc3: { key: 'b', sort: 3, v: 3 }, + doc3: { key: 'c', sort: 3, v: 3 }, doc4: { key: 'b', sort: 2, v: 2 }, doc5: { key: 'b', sort: 2, v: 1 }, doc6: { key: 'b', sort: 0, v: 0 } }; await withTestCollection(persistence, testDocs, async coll => { const snapshot1 = await getDocs( - query(coll, + query( + coll, or( - and(where('key', '==', 'b'), where('sort', '<=',2)), - and(where('key', '!=', 'b'), where('v',">",4)) - )) + and(where('key', '==', 'b'), where('sort', '<=', 2)), + and(where('key', '!=', 'b'), where('v', '>', 4)) + ) + ) ); - // Implicitly ordered by: 'sort' asc, 'v' asc, __name__ asc + // Implicitly ordered by: 'key' asc, 'sort' asc, 'v' asc, __name__ asc expect(toDataArray(snapshot1)).to.deep.equal([ { key: 'a', sort: 0, v: 5 }, { key: 'b', sort: 0, v: 0 }, + { key: 'b', sort: 2, v: 1 }, + { key: 'b', sort: 2, v: 2 } + ]); + + const snapshot2 = await getDocs( + query( + coll, + or( + and(where('key', '==', 'b'), where('sort', '<=', 2)), + and(where('key', '!=', 'b'), where('v', '>', 4)) + ), + orderBy('sort', 'desc'), + orderBy('key') + ) + ); + // Ordered by: 'sort' desc, 'key' asc, 'v' asc, __name__ asc + expect(toDataArray(snapshot2)).to.deep.equal([ { key: 'b', sort: 2, v: 1 }, { key: 'b', sort: 2, v: 2 }, + { key: 'a', sort: 0, v: 5 }, + { key: 'b', sort: 0, v: 0 } ]); - // const snapshot2 = await getDocs( - // query( - // coll, - // or( - // and(where('key', '==', 'b'), where('sort', '<=',2)), - // and(where('v', 'in', [2,3,4]), where('sort',">",3)) - // ), - // orderBy('sort', 'desc'), - // orderBy('key') - // ) - // ); - // expect(toDataArray(snapshot2)).to.deep.equal([ - // { key: 'aa', sort: 4, v: 4 }, - // { key: 'b', sort: 2, v: 2 }, - // { key: 'b', sort: 2, v: 1 }, - // { key: 'b', sort: 0, v: 0 }, - // ]); - - - // const snapshot3 = await getDocs( - // query( - // coll, - // and( - // or( - // and(where('a', '==', 'b'), where('c', '>=', 'd')), - // and(where('e', '==', 'f'), where('g', '!=', 'h')) - // ), - // or( - // and(where('i', '==', 'j'), where('k', '>', 'l')), - // and(where('m', '==', 'n'), where('o', '<', 'p')) - // ) - // ) - // )); - // expect(toDataArray(snapshot3)).to.deep.equal([ - // { key: 'aa', sort: 4, v: 4 }, - // { key: 'b', sort: 2, v: 2 }, - // { key: 'b', sort: 2, v: 1 }, - // { key: 'b', sort: 3, v: 3 } - // ]); + const snapshot3 = await getDocs( + query( + coll, + and( + or( + and(where('key', '==', 'b'), where('sort', '<=', 4)), + and(where('key', '!=', 'b'), where('v', '>', 4)) + ), + or( + and(where('key', '>', 'b'), where('sort', '>=', 1)), + and(where('key', '<', 'b'), where('v', '>', 0)) + ) + ) + ) + ); + expect(toDataArray(snapshot3)).to.deep.equal([ + { key: 'a', sort: 0, v: 5 } + ]); }); }); From fac7443272c321a2109ca6cab999a2f80c89e98c Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 12 Jul 2023 22:40:57 -0400 Subject: [PATCH 06/30] fix lint --- packages/firestore/src/core/query.ts | 4 ++-- packages/firestore/src/model/target_index_matcher.ts | 5 ++--- packages/firestore/test/integration/api/database.test.ts | 2 +- packages/firestore/test/integration/api/fields.test.ts | 2 +- packages/firestore/test/integration/api/query.test.ts | 3 ++- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index e10b621cf6a..21e668ddedd 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -181,7 +181,7 @@ export function getInequalityFilterFields(query: Query): FieldPath[] | null { function FiledPathLexicographicComparator( fieldPathA: FieldPath, fieldPathB: FieldPath -) { +): number { // Document key field should be ordered to the last. if (fieldPathA.isKeyField() && fieldPathB.isKeyField()) { return 0; @@ -251,7 +251,7 @@ export function queryOrderBy(query: Query): OrderBy[] { const queryImpl = debugCast(query, QueryImpl); if (queryImpl.memoizedOrderBy === null) { queryImpl.memoizedOrderBy = []; - const fieldsIncluded = new Set(); + const fieldsIncluded = new Set(); // Add the explicit order by. for (const orderBy of queryImpl.explicitOrderBy) { diff --git a/packages/firestore/src/model/target_index_matcher.ts b/packages/firestore/src/model/target_index_matcher.ts index 4dbeb734405..57ad49d44a0 100644 --- a/packages/firestore/src/model/target_index_matcher.ts +++ b/packages/firestore/src/model/target_index_matcher.ts @@ -18,7 +18,7 @@ import { FieldFilter, Operator } from '../core/filter'; import { Direction, OrderBy } from '../core/order_by'; import { Target } from '../core/target'; -import { debugAssert, hardAssert } from '../util/assert'; +import { hardAssert } from '../util/assert'; import { FieldIndex, @@ -27,7 +27,6 @@ import { IndexKind, IndexSegment } from './field_index'; -import { FieldPath } from './path'; /** * A light query planner for Firestore. @@ -53,7 +52,7 @@ export class TargetIndexMatcher { // The collection ID (or collection group) of the query target. private readonly collectionId: string; // The inequality filters of the target (if it exists). - private readonly inequalityFilters = new Map(); + private readonly inequalityFilters = new Map(); // The list of equality filters of the target. private readonly equalityFilters: FieldFilter[]; // The list of orderBys of the target. diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 1d72596fdf9..a68f8c35dc8 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -20,6 +20,7 @@ import { Deferred } from '@firebase/util'; import { expect, use } from 'chai'; import chaiAsPromised from 'chai-as-promised'; +import { ref } from '../../util/helpers'; import { EventsAccumulator } from '../util/events_accumulator'; import { addDoc, @@ -76,7 +77,6 @@ import { withNamedTestDbsOrSkipUnlessUsingEmulator } from '../util/helpers'; import { DEFAULT_SETTINGS, DEFAULT_PROJECT_ID } from '../util/settings'; -import { ref } from '../../util/helpers'; use(chaiAsPromised); diff --git a/packages/firestore/test/integration/api/fields.test.ts b/packages/firestore/test/integration/api/fields.test.ts index 306a7f51ddd..974264921b3 100644 --- a/packages/firestore/test/integration/api/fields.test.ts +++ b/packages/firestore/test/integration/api/fields.test.ts @@ -37,7 +37,7 @@ import { withTestDoc, withTestDocAndSettings } from '../util/helpers'; -import { DEFAULT_SETTINGS, USE_EMULATOR } from '../util/settings'; +import { DEFAULT_SETTINGS } from '../util/settings'; // Allow custom types for testing. // eslint-disable-next-line @typescript-eslint/no-explicit-any diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 709295a36ed..613b45471fe 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1396,6 +1396,7 @@ apiDescribe('Queries', persistence => { }); it('can use multiple inequality with nested field', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any const testData = (n?: number): any => { n = n || 1; return { @@ -1447,7 +1448,7 @@ apiDescribe('Queries', persistence => { }); }); - it.only('can use with nested composite filters', async () => { + it('can use with nested composite filters', async () => { const testDocs = { doc1: { key: 'a', sort: 0, v: 5 }, doc2: { key: 'aa', sort: 4, v: 4 }, From 832b29aceafbfe4efd9d588e9135f54862b38375 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 13 Jul 2023 13:01:58 -0400 Subject: [PATCH 07/30] add offline test and aggregation test --- .../firestore-compat/test/validation.test.ts | 53 ++-- .../test/integration/api/database.test.ts | 69 ++++- .../test/integration/api/query.test.ts | 248 ++++++++++++++++-- 3 files changed, 301 insertions(+), 69 deletions(-) diff --git a/packages/firestore-compat/test/validation.test.ts b/packages/firestore-compat/test/validation.test.ts index 0b94df7950e..d20ba67d34a 100644 --- a/packages/firestore-compat/test/validation.test.ts +++ b/packages/firestore-compat/test/validation.test.ts @@ -825,15 +825,11 @@ apiDescribe('Validation:', (persistence: boolean) => { } ); - validationIt(persistence, 'with different inequality fields fail', db => { + validationIt(persistence, 'can have different inequality fields', db => { const collection = db.collection('test'); expect(() => collection.where('x', '>=', 32).where('y', '<', 'cat') - ).to.throw( - 'Invalid query. All where filters with an ' + - 'inequality (<, <=, !=, not-in, >, or >=) must be on the same field.' + - ` But you have inequality filters on 'x' and 'y'` - ); + ).not.to.throw(); }); validationIt(persistence, 'with more than one != query fail', db => { @@ -845,59 +841,46 @@ apiDescribe('Validation:', (persistence: boolean) => { validationIt( persistence, - 'with != and inequality queries on different fields fail', + 'can have != and inequality queries on different fields', db => { const collection = db.collection('test'); expect(() => collection.where('y', '>', 32).where('x', '!=', 33) - ).to.throw( - 'Invalid query. All where filters with an ' + - 'inequality (<, <=, !=, not-in, >, or >=) must be on the same field.' + - ` But you have inequality filters on 'y' and 'x` - ); + ).not.to.throw(); } ); validationIt( persistence, - 'with != and inequality queries on different fields fail', + 'can have != and inequality queries on different fields', db => { const collection = db.collection('test'); expect(() => collection.where('y', '>', 32).where('x', 'not-in', [33]) - ).to.throw( - 'Invalid query. All where filters with an ' + - 'inequality (<, <=, !=, not-in, >, or >=) must be on the same field.' + - ` But you have inequality filters on 'y' and 'x` - ); + ).not.to.throw(); } ); validationIt( persistence, - 'with inequality different than first orderBy fail.', + 'can have inequality different than first orderBy', db => { const collection = db.collection('test'); - const reason = - `Invalid query. You have a where filter with an ` + - `inequality (<, <=, !=, not-in, >, or >=) on field 'x' and so you must also ` + - `use 'x' as your first argument to Query.orderBy(), but your first ` + - `orderBy() is on field 'y' instead.`; - expect(() => collection.where('x', '>', 32).orderBy('y')).to.throw( - reason - ); - expect(() => collection.orderBy('y').where('x', '>', 32)).to.throw( - reason - ); + expect(() => + collection.where('x', '>', 32).orderBy('y') + ).not.to.throw(); + expect(() => + collection.orderBy('y').where('x', '>', 32) + ).not.to.throw(); expect(() => collection.where('x', '>', 32).orderBy('y').orderBy('x') - ).to.throw(reason); + ).not.to.throw(); expect(() => collection.orderBy('y').orderBy('x').where('x', '>', 32) - ).to.throw(reason); - expect(() => collection.where('x', '!=', 32).orderBy('y')).to.throw( - reason - ); + ).not.to.throw(); + expect(() => + collection.where('x', '!=', 32).orderBy('y') + ).not.to.throw(); } ); diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index a68f8c35dc8..f78551ec025 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -20,7 +20,6 @@ import { Deferred } from '@firebase/util'; import { expect, use } from 'chai'; import chaiAsPromised from 'chai-as-promised'; -import { ref } from '../../util/helpers'; import { EventsAccumulator } from '../util/events_accumulator'; import { addDoc, @@ -643,18 +642,18 @@ apiDescribe('Database', persistence => { apiDescribe('Queries are validated client-side', persistence => { // NOTE: Failure cases are validated in validation_test.ts - it('inequality and equality on different fields works', () => { + it('same inequality fields works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => - query(coll, where('x', '>=', 32), where('y', '==', 'cat')) + query(coll, where('x', '>=', 32), where('x', '<=', 'cat')) ).not.to.throw(); }); }); - it('inequality on same field works', () => { + it('inequality and equality on different fields works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => - query(coll, where('x', '>=', 32), where('x', '<', 42)) + query(coll, where('x', '>=', 32), where('y', '==', 'cat')) ).not.to.throw(); }); }); @@ -670,11 +669,7 @@ apiDescribe('Database', persistence => { it('inequality with key fields works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => - query( - coll, - where(documentId(), '>=', ref('collection/id')), - where('x', '>=', 32) - ) + query(coll, where(documentId(), '>=', 'aa'), where('x', '>=', 32)) ).not.to.throw(); }); }); @@ -699,6 +694,32 @@ apiDescribe('Database', persistence => { }); }); + it('multiple inequality and array-contains on different fields works', () => { + return withTestCollection(persistence, {}, async coll => { + expect(() => + query( + coll, + where('x', '>=', 32), + where('y', 'array-contains', 'cat'), + where('z', '<=', 42) + ) + ).not.to.throw(); + }); + }); + + it('multiple inequality and array-contains-any on different fields works', () => { + return withTestCollection(persistence, {}, async coll => { + expect(() => + query( + coll, + where('x', '>=', 32), + where('y', 'array-contains-any', [1, 2]), + where('z', '<=', 42) + ) + ).not.to.throw(); + }); + }); + it('inequality and IN on different fields works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => @@ -707,6 +728,19 @@ apiDescribe('Database', persistence => { }); }); + it('multiple inequality and IN on different fields works', () => { + return withTestCollection(persistence, {}, async coll => { + expect(() => + query( + coll, + where('x', '>=', 32), + where('y', 'in', [1, 2]), + where('z', '<=', 42) + ) + ).not.to.throw(); + }); + }); + it('inequality and NOT IN on different fields works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => @@ -715,6 +749,19 @@ apiDescribe('Database', persistence => { }); }); + it('multiple inequality and NOT IN on different fields works', () => { + return withTestCollection(persistence, {}, async coll => { + expect(() => + query( + coll, + where('x', '>=', 32), + where('y', 'not-in', [1, 2]), + where('z', '<=', 42) + ) + ).not.to.throw(); + }); + }); + it('inequality same as orderBy works.', () => { return withTestCollection(persistence, {}, async coll => { expect(() => @@ -767,7 +814,7 @@ apiDescribe('Database', persistence => { }); }); - it('inequality different from orderBy works.', () => { + it('inequality different than orderBy works.', () => { return withTestCollection(persistence, {}, async coll => { expect(() => query(coll, where('x', '>', 32), orderBy('y')) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 0cfdb4bd4be..fd6c6e71972 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -23,9 +23,11 @@ import { EventsAccumulator } from '../util/events_accumulator'; import { addDoc, and, + average, Bytes, collection, collectionGroup, + count, deleteDoc, disableNetwork, doc, @@ -38,6 +40,8 @@ import { endBefore, FieldPath, GeoPoint, + getAggregateFromServer, + getCountFromServer, getDocs, getDocsFromCache, getDocsFromServer, @@ -53,6 +57,7 @@ import { setDoc, startAfter, startAt, + sum, Timestamp, updateDoc, where, @@ -71,6 +76,7 @@ import { } from '../util/helpers'; import { USE_EMULATOR } from '../util/settings'; import { captureExistenceFilterMismatches } from '../util/testing_hooks_util'; +import { getAggregate, getCount } from '../../../src/lite-api/aggregate'; apiDescribe('Queries', persistence => { addEqualityMatcher(); @@ -716,10 +722,10 @@ apiDescribe('Queries', persistence => { it('can query by document ID', () => { const testDocs = { - aa: { key: '1' }, - ab: { key: '2' }, - ba: { key: '3' }, - bb: { key: '4' } + aa: { key: 'aa' }, + ab: { key: 'ab' }, + ba: { key: 'ba' }, + bb: { key: 'bb' } }; return withTestCollection(persistence, testDocs, coll => { return getDocs(query(coll, where(documentId(), '==', 'ab'))) @@ -1341,56 +1347,93 @@ apiDescribe('Queries', persistence => { const testDocs = { doc1: { key: 'a', sort: 0, v: 0 }, doc2: { key: 'b', sort: 3, v: 1 }, - doc3: { key: 'c', sort: 1, v: 2 }, - doc4: { key: 'd', sort: 2, v: 3 } + doc3: { key: 'c', sort: 1, v: 3 }, + doc4: { key: 'd', sort: 2, v: 2 } }; - await withTestCollection(persistence, testDocs, async coll => { + return withTestCollection(persistence, testDocs, async coll => { // multiple inequality fields const snapshot1 = await getDocs( - query(coll, where('key', '!=', 'a'), where('sort', '<=', 2)) + query( + coll, + where('key', '!=', 'a'), + where('sort', '<=', 2), + where('v', '>', 2) + ) ); expect(toDataArray(snapshot1)).to.deep.equal([ - { key: 'c', sort: 1, v: 2 }, - { key: 'd', sort: 2, v: 3 } + { key: 'c', sort: 1, v: 3 } ]); + // duplicate inequality fields const snapshot2 = await getDocs( query( coll, where('key', '!=', 'a'), where('sort', '<=', 2), - where('v', '>', 2) + where('sort', '>', 1) ) ); expect(toDataArray(snapshot2)).to.deep.equal([ - { key: 'd', sort: 2, v: 3 } + { key: 'd', sort: 2, v: 2 } ]); - // duplicate inequality fields + // With NOT-IN operator const snapshot3 = await getDocs( query( coll, - where('key', '!=', 'a'), + where('key', '>=', 'a'), where('sort', '<=', 2), - where('sort', '>', 1) + where('v', 'not-in', [2, 4, 5]) ) ); expect(toDataArray(snapshot3)).to.deep.equal([ - { key: 'd', sort: 2, v: 3 } + { key: 'a', sort: 0, v: 0 }, + { key: 'c', sort: 1, v: 3 } ]); - // With NOT-IN operator + // With orderby const snapshot4 = await getDocs( query( coll, where('key', '>=', 'a'), where('sort', '<=', 2), - where('v', 'not-in', [2, 4, 5]) + orderBy('v', 'desc') ) ); expect(toDataArray(snapshot4)).to.deep.equal([ - { key: 'a', sort: 0, v: 0 }, - { key: 'd', sort: 2, v: 3 } + { key: 'c', sort: 1, v: 3 }, + { key: 'd', sort: 2, v: 2 }, + { key: 'a', sort: 0, v: 0 } + ]); + + // With limit + const snapshot5 = await getDocs( + query( + coll, + where('key', '>=', 'a'), + where('sort', '<=', 2), + orderBy('v', 'desc'), + limit(2) + ) + ); + expect(toDataArray(snapshot5)).to.deep.equal([ + { key: 'c', sort: 1, v: 3 }, + { key: 'd', sort: 2, v: 2 } + ]); + + // With limit to last + const snapshot6 = await getDocs( + query( + coll, + where('key', '>=', 'a'), + where('sort', '<=', 2), + orderBy('v', 'desc'), + limitToLast(2) + ) + ); + expect(toDataArray(snapshot6)).to.deep.equal([ + { key: 'd', sort: 2, v: 2 }, + { key: 'a', sort: 0, v: 0 } ]); }); }); @@ -1457,7 +1500,7 @@ apiDescribe('Queries', persistence => { doc5: { key: 'b', sort: 2, v: 1 }, doc6: { key: 'b', sort: 0, v: 0 } }; - await withTestCollection(persistence, testDocs, async coll => { + return withTestCollection(persistence, testDocs, async coll => { const snapshot1 = await getDocs( query( coll, @@ -1524,7 +1567,7 @@ apiDescribe('Queries', persistence => { doc5: { key: 'b', sort: 2, v: 1 }, doc6: { key: 'b', sort: 0, v: 0 } }; - await withTestCollection(persistence, testDocs, async coll => { + return withTestCollection(persistence, testDocs, async coll => { // Implicitly ordered by: 'key' asc, 'sort' asc, __name__ asc const snapshot1 = await getDocs( query( @@ -1569,7 +1612,7 @@ apiDescribe('Queries', persistence => { doc6: { key: 'c', sort: 0, v: 2 } }; - await withTestCollection(persistence, testDocs, async coll => { + return withTestCollection(persistence, testDocs, async coll => { // Ordered by: 'v' asc, 'key' asc, 'sort' asc, __name__ asc const snapshot1 = await getDocs( query( @@ -1638,6 +1681,165 @@ apiDescribe('Queries', persistence => { ]); }); }); + + it('can use in aggregate query', async () => { + const testDocs = { + doc1: { key: 'a', sort: 5, v: 0 }, + doc2: { key: 'aa', sort: 4, v: 0 }, + doc3: { key: 'b', sort: 3, v: 1 }, + doc4: { key: 'b', sort: 2, v: 1 }, + doc5: { key: 'bb', sort: 1, v: 1 } + }; + + return withTestCollection(persistence, testDocs, async coll => { + const snapshot1 = await getCountFromServer( + query( + coll, + where('key', '>', 'a'), + where('sort', '>=', 1), + orderBy('v') + ) + ); + expect(snapshot1.data().count).to.equal(4); + + const snapshot2 = await getCount( + query( + coll, + where('key', '>', 'a'), + where('sort', '>=', 1), + orderBy('v') + ) + ); + expect(snapshot2.data().count).to.equal(4); + + const snapshot3 = await getAggregateFromServer( + query( + coll, + where('key', '>', 'a'), + where('sort', '>=', 1), + where('v', '!=', 0) + ), + { + count: count(), + sum: sum('sort'), + avg: average('v') + } + ); + expect(snapshot3.data().count).to.equal(3); + expect(snapshot3.data().sum).to.equal(6); + expect(snapshot3.data().avg).to.equal(1); + + const snapshot4 = await getAggregate( + query( + coll, + where('key', '>', 'a'), + where('sort', '>=', 1), + where('v', '!=', 0) + ), + { + count: count(), + sum: sum('sort'), + avg: average('v') + } + ); + expect(snapshot4.data().count).to.equal(3); + expect(snapshot4.data().sum).to.equal(6); + expect(snapshot4.data().avg).to.equal(1); + }); + }); + + it('can use document ID im multiple inequality query', () => { + const testDocs = { + doc1: { key: 'a', sort: 5 }, + doc2: { key: 'aa', sort: 4 }, + doc3: { key: 'b', sort: 3 }, + doc4: { key: 'b', sort: 2 }, + doc5: { key: 'bb', sort: 1 } + }; + return withTestCollection(persistence, testDocs, async coll => { + const snapshot1 = await getDocs( + query( + coll, + where('sort', '>=', 1), + where('key', '!=', 'a'), + where(documentId(), '<', 'doc5') + ) + ); + // Document Key in inequality field will implicitly ordered to the last. + // Implicitly ordered by: 'key' asc, 'sort' asc, __name__ asc + expect(toDataArray(snapshot1)).to.deep.equal([ + { key: 'aa', sort: 4 }, + { key: 'b', sort: 2 }, + { key: 'b', sort: 3 } + ]); + + const snapshot2 = await getDocs( + query( + coll, + where(documentId(), '<', 'doc5'), + where('sort', '>=', 1), + where('key', '!=', 'a') + ) + ); + // Changing filters order will not effect implicit order. + // Implicitly ordered by: 'key' asc, 'sort' asc, __name__ asc + expect(toDataArray(snapshot2)).to.deep.equal([ + { key: 'aa', sort: 4 }, + { key: 'b', sort: 2 }, + { key: 'b', sort: 3 } + ]); + + const snapshot3 = await getDocs( + query( + coll, + where(documentId(), '<', 'doc5'), + where('sort', '>=', 1), + where('key', '!=', 'a'), + orderBy('sort', 'desc') + ) + ); + // Ordered by: 'sort' desc,'key' desc, __name__ desc + expect(toDataArray(snapshot3)).to.deep.equal([ + { key: 'aa', sort: 4 }, + { key: 'b', sort: 3 }, + { key: 'b', sort: 2 } + ]); + }); + }); + + it('can get documents while offline', () => { + const testDocs = { + doc1: { key: 'a', sort: 1 }, + doc2: { key: 'aa', sort: 4 }, + doc3: { key: 'b', sort: 3 }, + doc4: { key: 'b', sort: 2 } + }; + return withTestCollection( + persistence.toLruGc(), + testDocs, + async (coll, db) => { + const query_ = query( + coll, + where('key', '!=', 'a'), + where('sort', '<=', 3) + ); + //populate the cache. + const snapshot1 = await getDocs(query_); + expect(snapshot1.size).to.equal(2); + + await disableNetwork(db); + + const snapshot2 = await getDocs(query_); + expect(snapshot2.metadata.fromCache).to.be.true; + expect(snapshot2.metadata.hasPendingWrites).to.be.false; + // Implicitly ordered by: 'key' asc, 'sort' asc, __name__ asc + expect(toDataArray(snapshot2)).to.deep.equal([ + { key: 'b', sort: 2 }, + { key: 'b', sort: 3 } + ]); + } + ); + }); }); // OR Query tests only run when the SDK's local cache is configured to use From 9f4cef94aa756ee386643f4f812beb03d45c0acc Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 13 Jul 2023 14:23:48 -0400 Subject: [PATCH 08/30] format --- .../firestore-compat/test/validation.test.ts | 2 +- packages/firestore/src/core/filter.ts | 20 +++-- packages/firestore/src/core/query.ts | 50 ++++++------- .../test/integration/api/database.test.ts | 28 +------ .../test/integration/api/query.test.ts | 36 +-------- .../test/integration/api/validation.test.ts | 74 +++++++------------ 6 files changed, 64 insertions(+), 146 deletions(-) diff --git a/packages/firestore-compat/test/validation.test.ts b/packages/firestore-compat/test/validation.test.ts index d20ba67d34a..99ab4b63db5 100644 --- a/packages/firestore-compat/test/validation.test.ts +++ b/packages/firestore-compat/test/validation.test.ts @@ -852,7 +852,7 @@ apiDescribe('Validation:', (persistence: boolean) => { validationIt( persistence, - 'can have != and inequality queries on different fields', + 'can have NOT-IN and inequality queries on different fields', db => { const collection = db.collection('test'); expect(() => diff --git a/packages/firestore/src/core/filter.ts b/packages/firestore/src/core/filter.ts index 918828f313a..6787860b457 100644 --- a/packages/firestore/src/core/filter.ts +++ b/packages/firestore/src/core/filter.ts @@ -57,7 +57,7 @@ export abstract class Filter { abstract getFilters(): Filter[]; - abstract getInequalityFilters(): readonly FieldFilter[] | null; + abstract getInequalityFilters(): readonly FieldFilter[]; } export class FieldFilter extends Filter { @@ -195,11 +195,11 @@ export class FieldFilter extends Filter { return [this]; } - getInequalityFilters(): readonly FieldFilter[] | null { + getInequalityFilters(): readonly FieldFilter[] { if (this.isInequality()) { return [this]; } - return null; + return []; } } @@ -248,17 +248,15 @@ export class CompositeFilter extends Filter { } // Performs a depth-first search to find and return the inequality FieldFilters in the composite - // filter. Returns `null` if none of the FieldFilters satisfy the predicate. - getInequalityFilters(): readonly FieldFilter[] | null { + // filter. Returns an empty array if none of the FieldFilters has inequality filters. + getInequalityFilters(): readonly FieldFilter[] { const result: FieldFilter[] = []; - this.getFlattenedFilters().forEach((fieldFilter: FieldFilter) => { - const subFilters = fieldFilter.getInequalityFilters(); - if (subFilters !== null) { - result.push(...subFilters); - } + this.getFlattenedFilters().forEach((filter: FieldFilter) => { + result.push(...filter.getInequalityFilters()); }); - return result.length === 0 ? null : result; + + return result; } } diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index 21e668ddedd..781a8562232 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -165,27 +165,29 @@ export function queryMatchesAllDocuments(query: Query): boolean { ); } -export function getInequalityFilterFields(query: Query): FieldPath[] | null { +// Perform an in depth search on the query to get all inequality filter fields. Returns an empty +// array if there is no inequality filters. +export function getInequalityFilterFields(query: Query): FieldPath[] { const result: FieldPath[] = []; query.filters.forEach((filter: Filter) => { - const inequalityFilters = filter.getInequalityFilters(); - if (inequalityFilters !== null) { - result.push(...inequalityFilters.map(filter => filter.field)); - } + const inequalityFields = filter + .getInequalityFilters() + .map(filter => filter.field); + result.push(...inequalityFields); }); - return result.length === 0 ? null : result; + return result; } function FiledPathLexicographicComparator( fieldPathA: FieldPath, fieldPathB: FieldPath ): number { - // Document key field should be ordered to the last. if (fieldPathA.isKeyField() && fieldPathB.isKeyField()) { return 0; } else if (fieldPathA.isKeyField() || fieldPathB.isKeyField()) { + // Document key field should always be ordered to the last. return fieldPathA.isKeyField() ? 1 : -1; } @@ -204,7 +206,7 @@ function FiledPathLexicographicComparator( } } - // If the common segments are the same, compare the lengths + // If the common segments are the same, compare the lengths. if (segmentsA.length < segmentsB.length) { return -1; } else if (segmentsA.length > segmentsB.length) { @@ -243,9 +245,9 @@ export function isCollectionGroupQuery(query: Query): boolean { } /** - * Returns the implicit order by constraint that is used to execute the Query, - * which can be different from the order by constraints the user provided (e.g. - * the SDK and backend always orders by `__name__`). + * Returns the order by constraint that is used to execute the Query, in which the implicit order by + * constraint can be different from the order by constraints the user provided (e.g. the SDK and + * backend always orders by `__name__`). */ export function queryOrderBy(query: Query): OrderBy[] { const queryImpl = debugCast(query, QueryImpl); @@ -253,35 +255,31 @@ export function queryOrderBy(query: Query): OrderBy[] { queryImpl.memoizedOrderBy = []; const fieldsIncluded = new Set(); - // Add the explicit order by. + // Any explicit order by fields should be added as is. for (const orderBy of queryImpl.explicitOrderBy) { queryImpl.memoizedOrderBy.push(orderBy); fieldsIncluded.add(orderBy.field.canonicalString()); } - // The order of the implicit key ordering always matches the last - // explicit order by. + // The order of the implicit ordering always matches the last explicit order by. const lastDirection = queryImpl.explicitOrderBy.length > 0 ? queryImpl.explicitOrderBy[queryImpl.explicitOrderBy.length - 1].dir : Direction.ASCENDING; + // Any inequality field not explicitly ordered should be implicitly ordered in a lexicographical + // order. When there are multiple inequality filters on the same field, the field should be added + // only once. const inequalityFields = getInequalityFilterFields(queryImpl); - - // Any inequality field not explicitly ordered should be implicitly ordered in a lexicographical order. - if (inequalityFields !== null) { - inequalityFields.sort(FiledPathLexicographicComparator); - for (const inequalityField of inequalityFields) { - if (!fieldsIncluded.has(inequalityField.canonicalString())) { - queryImpl.memoizedOrderBy.push( - new OrderBy(inequalityField, lastDirection) - ); - fieldsIncluded.add(inequalityField.canonicalString()); - } + inequalityFields.sort(FiledPathLexicographicComparator); + for (const field of inequalityFields) { + if (!fieldsIncluded.has(field.canonicalString())) { + queryImpl.memoizedOrderBy.push(new OrderBy(field, lastDirection)); + fieldsIncluded.add(field.canonicalString()); } } - // Add the document key to the last if it is not included. + // Add the document key field to the last if it is not included above. if (!fieldsIncluded.has(FieldPath.keyField().canonicalString())) { queryImpl.memoizedOrderBy.push( new OrderBy(FieldPath.keyField(), lastDirection) diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index f78551ec025..636535bc983 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -784,28 +784,6 @@ apiDescribe('Database', persistence => { }); }); - it('inequality same as first orderBy works.', () => { - return withTestCollection(persistence, {}, async coll => { - expect(() => - query(coll, where('x', '>', 32), orderBy('x'), orderBy('y')) - ).not.to.throw(); - expect(() => - query(coll, orderBy('x'), where('x', '>', 32), orderBy('y')) - ).not.to.throw(); - }); - }); - - it('!= same as first orderBy works.', () => { - return withTestCollection(persistence, {}, async coll => { - expect(() => - query(coll, where('x', '!=', 32), orderBy('x'), orderBy('y')) - ).not.to.throw(); - expect(() => - query(coll, orderBy('x'), where('x', '!=', 32), orderBy('y')) - ).not.to.throw(); - }); - }); - it('equality different than orderBy works', () => { return withTestCollection(persistence, {}, async coll => { expect(() => @@ -822,11 +800,6 @@ apiDescribe('Database', persistence => { expect(() => query(coll, orderBy('y'), where('x', '>', 32)) ).not.to.throw(); - }); - }); - - it('inequality different from first orderBy works.', () => { - return withTestCollection(persistence, {}, async coll => { expect(() => query(coll, where('x', '>', 32), orderBy('y'), orderBy('z')) ).not.to.throw(); @@ -835,6 +808,7 @@ apiDescribe('Database', persistence => { ).not.to.throw(); }); }); + it('multiple inequality different from orderBy works.', () => { return withTestCollection(persistence, {}, async coll => { expect(() => diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 475a7c6fb0b..7de4e619fd9 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -75,7 +75,6 @@ import { } from '../util/helpers'; import { USE_EMULATOR } from '../util/settings'; import { captureExistenceFilterMismatches } from '../util/testing_hooks_util'; -import { getAggregate, getCount } from '../../../src/lite-api/aggregate'; apiDescribe('Queries', persistence => { addEqualityMatcher(); @@ -1701,34 +1700,7 @@ apiDescribe('Queries', persistence => { ); expect(snapshot1.data().count).to.equal(4); - const snapshot2 = await getCount( - query( - coll, - where('key', '>', 'a'), - where('sort', '>=', 1), - orderBy('v') - ) - ); - expect(snapshot2.data().count).to.equal(4); - - const snapshot3 = await getAggregateFromServer( - query( - coll, - where('key', '>', 'a'), - where('sort', '>=', 1), - where('v', '!=', 0) - ), - { - count: count(), - sum: sum('sort'), - avg: average('v') - } - ); - expect(snapshot3.data().count).to.equal(3); - expect(snapshot3.data().sum).to.equal(6); - expect(snapshot3.data().avg).to.equal(1); - - const snapshot4 = await getAggregate( + const snapshot2 = await getAggregateFromServer( query( coll, where('key', '>', 'a'), @@ -1741,9 +1713,9 @@ apiDescribe('Queries', persistence => { avg: average('v') } ); - expect(snapshot4.data().count).to.equal(3); - expect(snapshot4.data().sum).to.equal(6); - expect(snapshot4.data().avg).to.equal(1); + expect(snapshot2.data().count).to.equal(3); + expect(snapshot2.data().sum).to.equal(6); + expect(snapshot2.data().avg).to.equal(1); }); }); diff --git a/packages/firestore/test/integration/api/validation.test.ts b/packages/firestore/test/integration/api/validation.test.ts index c65e54da753..3e6c68683f1 100644 --- a/packages/firestore/test/integration/api/validation.test.ts +++ b/packages/firestore/test/integration/api/validation.test.ts @@ -1112,6 +1112,30 @@ apiDescribe('Validation:', persistence => { ).to.throw( "Invalid query. You cannot use 'not-in' filters with 'in' filters." ); + + // Multiple top level composite filters + expect(() => + // @ts-ignore + query(coll, and(where('a', '==', 'b')), or(where('b', '==', 'a'))) + ).to.throw( + 'InvalidQuery. When using composite filters, you cannot use ' + + 'more than one filter at the top level. Consider nesting the multiple ' + + 'filters within an `and(...)` statement. For example: ' + + 'change `query(query, where(...), or(...))` to ' + + '`query(query, and(where(...), or(...)))`.' + ); + + // Once top level composite filter and one top level field filter + expect(() => + // @ts-ignore + query(coll, or(where('a', '==', 'b')), where('b', '==', 'a')) + ).to.throw( + 'InvalidQuery. When using composite filters, you cannot use ' + + 'more than one filter at the top level. Consider nesting the multiple ' + + 'filters within an `and(...)` statement. For example: ' + + 'change `query(query, where(...), or(...))` to ' + + '`query(query, and(where(...), or(...)))`.' + ); }); validationIt( @@ -1183,7 +1207,7 @@ apiDescribe('Validation:', persistence => { validationIt( persistence, - 'can have inequality different than first orderBy', + 'can have inequality different than orderBy', db => { const coll = collection(db, 'test'); // single inequality @@ -1290,54 +1314,6 @@ apiDescribe('Validation:', persistence => { ) ) ).not.to.throw(); - - // Multiple top level composite filters - expect(() => - // @ts-ignore - query(coll, and(where('a', '==', 'b')), or(where('b', '==', 'a'))) - ).to.throw( - 'InvalidQuery. When using composite filters, you cannot use ' + - 'more than one filter at the top level. Consider nesting the multiple ' + - 'filters within an `and(...)` statement. For example: ' + - 'change `query(query, where(...), or(...))` to ' + - '`query(query, and(where(...), or(...)))`.' - ); - - // Once top level composite filter and one top level field filter - expect(() => - // @ts-ignore - query(coll, or(where('a', '==', 'b')), where('b', '==', 'a')) - ).to.throw( - 'InvalidQuery. When using composite filters, you cannot use ' + - 'more than one filter at the top level. Consider nesting the multiple ' + - 'filters within an `and(...)` statement. For example: ' + - 'change `query(query, where(...), or(...))` to ' + - '`query(query, and(where(...), or(...)))`.' - ); - - // Multiple top level composite filters - expect(() => - // @ts-ignore - query(coll, and(where('a', '==', 'b')), or(where('b', '==', 'a'))) - ).to.throw( - 'InvalidQuery. When using composite filters, you cannot use ' + - 'more than one filter at the top level. Consider nesting the multiple ' + - 'filters within an `and(...)` statement. For example: ' + - 'change `query(query, where(...), or(...))` to ' + - '`query(query, and(where(...), or(...)))`.' - ); - - // Once top level composite filter and one top level field filter - expect(() => - // @ts-ignore - query(coll, or(where('a', '==', 'b')), where('b', '==', 'a')) - ).to.throw( - 'InvalidQuery. When using composite filters, you cannot use ' + - 'more than one filter at the top level. Consider nesting the multiple ' + - 'filters within an `and(...)` statement. For example: ' + - 'change `query(query, where(...), or(...))` to ' + - '`query(query, and(where(...), or(...)))`.' - ); } ); }); From 74dc8afa5f4e05b100a2a0f98115b0c2a7eb86cd Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 13 Jul 2023 22:06:08 -0400 Subject: [PATCH 09/30] add more tests --- .../test/integration/api/database.test.ts | 27 ++++++ .../test/integration/api/query.test.ts | 96 +++++++++++++++---- 2 files changed, 107 insertions(+), 16 deletions(-) diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 636535bc983..3ce1b1c7727 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1139,6 +1139,33 @@ apiDescribe('Database', persistence => { }); }); + it('can compare multiple inequality Query instances with isEqual().', () => { + return withTestDb(persistence, async firestore => { + const query1 = query( + collection(firestore, 'foo'), + where('x', '>=', 42), + where('y', '!=', 42), + orderBy('z') + ); + const query2 = query( + collection(firestore, 'foo'), + where('x', '>=', 42), + where('y', '!=', 42), + orderBy('z') + ); + expect(queryEqual(query1, query2)).to.be.true; + + // Inequality fields in different order + const query3 = query( + collection(firestore, 'foo'), + where('y', '!=', 42), + where('x', '>=', 42), + orderBy('z') + ); + expect(queryEqual(query1, query3)).to.be.false; + }); + }); + it('can traverse collections and documents.', () => { return withTestDb(persistence, async db => { const expected = 'a/b/c/d'; diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 7de4e619fd9..c64b685df35 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1375,22 +1375,36 @@ apiDescribe('Queries', persistence => { { key: 'd', sort: 2, v: 2 } ]); - // With NOT-IN operator + // With multiple IN const snapshot3 = await getDocs( query( coll, where('key', '>=', 'a'), where('sort', '<=', 2), - where('v', 'not-in', [2, 4, 5]) + where('v', 'in', [2, 3, 4]), + where('sort', 'in', [2, 3]) ) ); expect(toDataArray(snapshot3)).to.deep.equal([ + { key: 'd', sort: 2, v: 2 } + ]); + + // With NOT-IN + const snapshot4 = await getDocs( + query( + coll, + where('key', '>=', 'a'), + where('sort', '<=', 2), + where('v', 'not-in', [2, 4, 5]) + ) + ); + expect(toDataArray(snapshot4)).to.deep.equal([ { key: 'a', sort: 0, v: 0 }, { key: 'c', sort: 1, v: 3 } ]); // With orderby - const snapshot4 = await getDocs( + const snapshot5 = await getDocs( query( coll, where('key', '>=', 'a'), @@ -1398,14 +1412,14 @@ apiDescribe('Queries', persistence => { orderBy('v', 'desc') ) ); - expect(toDataArray(snapshot4)).to.deep.equal([ + expect(toDataArray(snapshot5)).to.deep.equal([ { key: 'c', sort: 1, v: 3 }, { key: 'd', sort: 2, v: 2 }, { key: 'a', sort: 0, v: 0 } ]); // With limit - const snapshot5 = await getDocs( + const snapshot6 = await getDocs( query( coll, where('key', '>=', 'a'), @@ -1414,13 +1428,13 @@ apiDescribe('Queries', persistence => { limit(2) ) ); - expect(toDataArray(snapshot5)).to.deep.equal([ + expect(toDataArray(snapshot6)).to.deep.equal([ { key: 'c', sort: 1, v: 3 }, { key: 'd', sort: 2, v: 2 } ]); // With limit to last - const snapshot6 = await getDocs( + const snapshot7 = await getDocs( query( coll, where('key', '>=', 'a'), @@ -1429,14 +1443,50 @@ apiDescribe('Queries', persistence => { limitToLast(2) ) ); - expect(toDataArray(snapshot6)).to.deep.equal([ + expect(toDataArray(snapshot7)).to.deep.equal([ { key: 'd', sort: 2, v: 2 }, { key: 'a', sort: 0, v: 0 } ]); }); }); - it('can use multiple inequality with nested field', () => { + it('can use with array membership', async () => { + const testDocs = { + doc1: { key: 'a', sort: 0, v: [0] }, + doc2: { key: 'b', sort: 1, v: [0, 1, 3] }, + doc3: { key: 'c', sort: 1, v: [] }, + doc4: { key: 'd', sort: 2, v: [1] }, + doc5: { key: 'e', sort: 3, v: [2, 4] } + }; + return withTestCollection(persistence, testDocs, async coll => { + const snapshot1 = await getDocs( + query( + coll, + where('key', '!=', 'a'), + where('sort', '>=', 1), + where('v', 'array-contains', 0) + ) + ); + expect(toDataArray(snapshot1)).to.deep.equal([ + { key: 'b', sort: 1, v: [0, 1, 3] } + ]); + + const snapshot2 = await getDocs( + query( + coll, + where('key', '!=', 'a'), + where('sort', '>=', 1), + where('v', 'array-contains-any', [0, 1]) + ) + ); + expect(toDataArray(snapshot2)).to.deep.equal([ + { key: 'b', sort: 1, v: [0, 1, 3] }, + { key: 'd', sort: 2, v: [1] } + ]); + }); + }); + + it('can use with nested field', () => { // eslint-disable-next-line @typescript-eslint/no-explicit-any const testData = (n?: number): any => { n = n || 1; @@ -1848,6 +1898,20 @@ apiDescribe('Queries', persistence => { 'doc4' ); + // explicit AND: a != 1 && b < 2 + await checkOnlineAndOfflineResultsMatch( + query(coll, and(where('a', '!=', 1), where('b', '<', 2))), + 'doc2' + ); + + // explicit AND: a < 3 && b not-in [2 + await checkOnlineAndOfflineResultsMatch( + query(coll, and(where('a', '<', 3), where('b', 'not-in', [2, 3]))), + 'doc1', + 'doc5', + 'doc2' + ); + // a == 1, limit 2 await checkOnlineAndOfflineResultsMatch( query(coll, where('a', '==', 1), limit(2)), @@ -2174,13 +2238,6 @@ apiDescribe('Queries', persistence => { 'doc3' ); - // with multiple inequality: a>2 || b<=1. - await checkOnlineAndOfflineResultsMatch( - query(coll, or(where('a', '>', 2), where('b', '<', 1))), - 'doc1', - 'doc3' - ); - // Test with limits (implicit order by ASC): (a==1) || (b > 0) LIMIT 2 await checkOnlineAndOfflineResultsMatch( query(coll, or(where('a', '==', 1), where('b', '>', 0)), limit(2)), @@ -2222,6 +2279,13 @@ apiDescribe('Queries', persistence => { ), 'doc2' ); + + // with multiple inequality: a>2 || b<=1. + await checkOnlineAndOfflineResultsMatch( + query(coll, or(where('a', '>', 2), where('b', '<', 1))), + 'doc1', + 'doc3' + ); }); }); From d140edf1203e8349bb38343d56aaf92ded620bef Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 14 Jul 2023 13:18:11 -0400 Subject: [PATCH 10/30] re-organize tests and format --- .../test/integration/api/query.test.ts | 125 ++++++++++++++---- 1 file changed, 100 insertions(+), 25 deletions(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index c64b685df35..9283aba470d 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1349,7 +1349,7 @@ apiDescribe('Queries', persistence => { doc4: { key: 'd', sort: 2, v: 2 } }; return withTestCollection(persistence, testDocs, async coll => { - // multiple inequality fields + // Multiple inequality fields const snapshot1 = await getDocs( query( coll, @@ -1362,7 +1362,7 @@ apiDescribe('Queries', persistence => { { key: 'c', sort: 1, v: 3 } ]); - // duplicate inequality fields + // Duplicate inequality fields const snapshot2 = await getDocs( query( coll, @@ -1433,7 +1433,7 @@ apiDescribe('Queries', persistence => { { key: 'd', sort: 2, v: 2 } ]); - // With limit to last + // With limitToLast const snapshot7 = await getDocs( query( coll, @@ -1450,13 +1450,46 @@ apiDescribe('Queries', persistence => { }); }); + it('can use on unary values', async () => { + const testDocs = { + doc1: { key: 'a', sort: 0, v: 0 }, + doc2: { key: 'b', sort: NaN, v: 1 }, + doc3: { key: 'c', sort: null, v: 3 }, + doc4: { key: 'd', v: 0 }, + doc5: { key: 'e', sort: 1 }, + doc6: { key: 'f', sort: 1, v: 1 } + }; + return withTestCollection(persistence, testDocs, async coll => { + const snapshot1 = await getDocs( + query(coll, where('key', '!=', 'a'), where('sort', '<=', 2)) + ); + expect(toDataArray(snapshot1)).to.deep.equal([ + { key: 'e', sort: 1 }, + { key: 'f', sort: 1, v: 1 } + ]); + + const snapshot2 = await getDocs( + query( + coll, + where('key', '!=', 'a'), + where('sort', '<=', 2), + where('v', '<=', 1) + ) + ); + expect(toDataArray(snapshot2)).to.deep.equal([ + { key: 'f', sort: 1, v: 1 } + ]); + }); + }); it('can use with array membership', async () => { const testDocs = { doc1: { key: 'a', sort: 0, v: [0] }, doc2: { key: 'b', sort: 1, v: [0, 1, 3] }, doc3: { key: 'c', sort: 1, v: [] }, doc4: { key: 'd', sort: 2, v: [1] }, - doc5: { key: 'e', sort: 3, v: [2, 4] } + doc5: { key: 'e', sort: 3, v: [2, 4] }, + doc6: { key: 'f', sort: 4, v: [NaN] }, + doc7: { key: 'g', sort: 4, v: [null] } }; return withTestCollection(persistence, testDocs, async coll => { const snapshot1 = await getDocs( @@ -1861,6 +1894,69 @@ apiDescribe('Queries', persistence => { } ); }); + + // eslint-disable-next-line no-restricted-properties + (persistence.gc === 'lru' ? it : it.skip)( + 'can get same result from server and cache', + () => { + const testDocs = { + doc1: { a: 1, b: 0 }, + doc2: { a: 2, b: 1 }, + doc3: { a: 3, b: 2 }, + doc4: { a: 1, b: 3 }, + doc5: { a: 1, b: 1 } + }; + + return withTestCollection(persistence, testDocs, async coll => { + // implicit AND: a != 1 && b < 2 + await checkOnlineAndOfflineResultsMatch( + query(coll, where('a', '!=', 1), where('b', '<', 2)), + 'doc2' + ); + + // explicit AND: a != 1 && b < 2 + await checkOnlineAndOfflineResultsMatch( + query(coll, and(where('a', '!=', 1), where('b', '<', 2))), + 'doc2' + ); + + // explicit AND: a < 3 && b not-in [2, 3] + await checkOnlineAndOfflineResultsMatch( + query(coll, and(where('a', '<', 3), where('b', 'not-in', [2, 3]))), + 'doc1', + 'doc5', + 'doc2' + ); + + // a <3 && b != 0, implicitly ordered by: a asc, b asc, __name__ asc + await checkOnlineAndOfflineResultsMatch( + query(coll, where('a', '<', 3), where('b', '!=', 0), limit(2)), + 'doc5', + 'doc4' + ); + + // a <3 && b != 0, implicitly ordered by: b desc, a desc, __name__ desc + await checkOnlineAndOfflineResultsMatch( + query( + coll, + where('a', '<', 3), + where('b', '!=', 0), + orderBy('b', 'desc'), + limit(2) + ), + 'doc4', + 'doc2' + ); + + // explicit OR: multiple inequality: a>2 || b<=1. + await checkOnlineAndOfflineResultsMatch( + query(coll, or(where('a', '>', 2), where('b', '<', 1))), + 'doc1', + 'doc3' + ); + }); + } + ); }); // OR Query tests only run when the SDK's local cache is configured to use @@ -1898,20 +1994,6 @@ apiDescribe('Queries', persistence => { 'doc4' ); - // explicit AND: a != 1 && b < 2 - await checkOnlineAndOfflineResultsMatch( - query(coll, and(where('a', '!=', 1), where('b', '<', 2))), - 'doc2' - ); - - // explicit AND: a < 3 && b not-in [2 - await checkOnlineAndOfflineResultsMatch( - query(coll, and(where('a', '<', 3), where('b', 'not-in', [2, 3]))), - 'doc1', - 'doc5', - 'doc2' - ); - // a == 1, limit 2 await checkOnlineAndOfflineResultsMatch( query(coll, where('a', '==', 1), limit(2)), @@ -2279,13 +2361,6 @@ apiDescribe('Queries', persistence => { ), 'doc2' ); - - // with multiple inequality: a>2 || b<=1. - await checkOnlineAndOfflineResultsMatch( - query(coll, or(where('a', '>', 2), where('b', '<', 1))), - 'doc1', - 'doc3' - ); }); }); From c444386758911cd4e2f27abf14d2d6dedc4af97c Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 20 Jul 2023 12:03:35 -0400 Subject: [PATCH 11/30] resolve comments --- packages/firestore/src/core/filter.ts | 10 +++------- packages/firestore/src/core/query.ts | 6 +++--- packages/firestore/src/model/target_index_matcher.ts | 1 + 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/packages/firestore/src/core/filter.ts b/packages/firestore/src/core/filter.ts index 6787860b457..adc187fef12 100644 --- a/packages/firestore/src/core/filter.ts +++ b/packages/firestore/src/core/filter.ts @@ -250,13 +250,9 @@ export class CompositeFilter extends Filter { // Performs a depth-first search to find and return the inequality FieldFilters in the composite // filter. Returns an empty array if none of the FieldFilters has inequality filters. getInequalityFilters(): readonly FieldFilter[] { - const result: FieldFilter[] = []; - - this.getFlattenedFilters().forEach((filter: FieldFilter) => { - result.push(...filter.getInequalityFilters()); - }); - - return result; + return this.getFlattenedFilters().filter((filter: FieldFilter) => + filter.isInequality() + ); } } diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index 781a8562232..10a79c27f1e 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -180,7 +180,7 @@ export function getInequalityFilterFields(query: Query): FieldPath[] { return result; } -function FiledPathLexicographicComparator( +function FieldPathLexicographicComparator( fieldPathA: FieldPath, fieldPathB: FieldPath ): number { @@ -245,7 +245,7 @@ export function isCollectionGroupQuery(query: Query): boolean { } /** - * Returns the order by constraint that is used to execute the Query, in which the implicit order by + * Returns the order by constraint that is used to execute the Query. Note that the implicit order by * constraint can be different from the order by constraints the user provided (e.g. the SDK and * backend always orders by `__name__`). */ @@ -271,7 +271,7 @@ export function queryOrderBy(query: Query): OrderBy[] { // order. When there are multiple inequality filters on the same field, the field should be added // only once. const inequalityFields = getInequalityFilterFields(queryImpl); - inequalityFields.sort(FiledPathLexicographicComparator); + inequalityFields.sort(FieldPathLexicographicComparator); for (const field of inequalityFields) { if (!fieldsIncluded.has(field.canonicalString())) { queryImpl.memoizedOrderBy.push(new OrderBy(field, lastDirection)); diff --git a/packages/firestore/src/model/target_index_matcher.ts b/packages/firestore/src/model/target_index_matcher.ts index 57ad49d44a0..61752ea3cb2 100644 --- a/packages/firestore/src/model/target_index_matcher.ts +++ b/packages/firestore/src/model/target_index_matcher.ts @@ -148,6 +148,7 @@ export class TargetIndexMatcher { return false; } + // Only a single inequality is currently supported. Get the only entry in the map. const inequalityFilter = this.inequalityFilters.values().next().value; // 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 From 2783ef4d762c56ffad5661e3ba28753b474c40fe Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 20 Jul 2023 12:31:37 -0400 Subject: [PATCH 12/30] use sortedset --- packages/firestore/src/core/query.ts | 75 +++++++--------------------- 1 file changed, 19 insertions(+), 56 deletions(-) diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index 10a79c27f1e..3d3ee4f6aef 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -19,6 +19,7 @@ import { compareDocumentsByField, Document } from '../model/document'; import { DocumentKey } from '../model/document_key'; import { FieldPath, ResourcePath } from '../model/path'; import { debugAssert, debugCast, fail } from '../util/assert'; +import { SortedSet } from '../util/sorted_set'; import { Bound, @@ -165,57 +166,20 @@ export function queryMatchesAllDocuments(query: Query): boolean { ); } -// Perform an in depth search on the query to get all inequality filter fields. Returns an empty -// array if there is no inequality filters. -export function getInequalityFilterFields(query: Query): FieldPath[] { - const result: FieldPath[] = []; - +// Returns the sorted set of inequality filter fields used in this query. +export function getInequalityFilterFields(query: Query): SortedSet { + let result = new SortedSet(FieldPath.comparator); query.filters.forEach((filter: Filter) => { const inequalityFields = filter .getInequalityFilters() .map(filter => filter.field); - result.push(...inequalityFields); + inequalityFields.forEach((field: FieldPath) => { + result = result.add(field); + }); }); - return result; } -function FieldPathLexicographicComparator( - fieldPathA: FieldPath, - fieldPathB: FieldPath -): number { - if (fieldPathA.isKeyField() && fieldPathB.isKeyField()) { - return 0; - } else if (fieldPathA.isKeyField() || fieldPathB.isKeyField()) { - // Document key field should always be ordered to the last. - return fieldPathA.isKeyField() ? 1 : -1; - } - - const segmentsA = fieldPathA.toArray(); - const segmentsB = fieldPathB.toArray(); - const minLength = Math.min(segmentsA.length, segmentsB.length); - - for (let i = 0; i < minLength; i++) { - const segmentA = segmentsA[i]; - const segmentB = segmentsB[i]; - - if (segmentA < segmentB) { - return -1; - } else if (segmentA > segmentB) { - return 1; - } - } - - // If the common segments are the same, compare the lengths. - if (segmentsA.length < segmentsB.length) { - return -1; - } else if (segmentsA.length > segmentsB.length) { - return 1; - } - - return 0; -} - /** * Creates a new Query for a collection group query that matches all documents * within the provided collection group. @@ -253,12 +217,12 @@ export function queryOrderBy(query: Query): OrderBy[] { const queryImpl = debugCast(query, QueryImpl); if (queryImpl.memoizedOrderBy === null) { queryImpl.memoizedOrderBy = []; - const fieldsIncluded = new Set(); + const explicitFields = new Set(); // Any explicit order by fields should be added as is. for (const orderBy of queryImpl.explicitOrderBy) { queryImpl.memoizedOrderBy.push(orderBy); - fieldsIncluded.add(orderBy.field.canonicalString()); + explicitFields.add(orderBy.field.canonicalString()); } // The order of the implicit ordering always matches the last explicit order by. @@ -267,26 +231,25 @@ export function queryOrderBy(query: Query): OrderBy[] { ? queryImpl.explicitOrderBy[queryImpl.explicitOrderBy.length - 1].dir : Direction.ASCENDING; - // Any inequality field not explicitly ordered should be implicitly ordered in a lexicographical + // Any inequality fields not explicitly ordered should be implicitly ordered in a lexicographical // order. When there are multiple inequality filters on the same field, the field should be added // only once. - const inequalityFields = getInequalityFilterFields(queryImpl); - inequalityFields.sort(FieldPathLexicographicComparator); - for (const field of inequalityFields) { - if (!fieldsIncluded.has(field.canonicalString())) { - queryImpl.memoizedOrderBy.push(new OrderBy(field, lastDirection)); - fieldsIncluded.add(field.canonicalString()); + // Note: key field would be lexicographically ordered to the first by sorted set. + const inequalityFields: SortedSet = + getInequalityFilterFields(queryImpl); + inequalityFields.forEach(field => { + if (!explicitFields.has(field.canonicalString()) && !field.isKeyField()) { + queryImpl.memoizedOrderBy!.push(new OrderBy(field, lastDirection)); } - } + }); - // Add the document key field to the last if it is not included above. - if (!fieldsIncluded.has(FieldPath.keyField().canonicalString())) { + // Add the document key field to the last if it is not explicitly ordered. + if (!explicitFields.has(FieldPath.keyField().canonicalString())) { queryImpl.memoizedOrderBy.push( new OrderBy(FieldPath.keyField(), lastDirection) ); } } - return queryImpl.memoizedOrderBy; } From 5b1fdb52b78d6804023b859116583fbf06adc142 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 20 Jul 2023 14:25:51 -0400 Subject: [PATCH 13/30] replace test docs to ids --- .../test/integration/api/query.test.ts | 182 +++++++----------- 1 file changed, 66 insertions(+), 116 deletions(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 9283aba470d..11d1ba000b6 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1358,9 +1358,7 @@ apiDescribe('Queries', persistence => { where('v', '>', 2) ) ); - expect(toDataArray(snapshot1)).to.deep.equal([ - { key: 'c', sort: 1, v: 3 } - ]); + expect(toIds(snapshot1)).to.deep.equal(['doc3']); // Duplicate inequality fields const snapshot2 = await getDocs( @@ -1371,9 +1369,7 @@ apiDescribe('Queries', persistence => { where('sort', '>', 1) ) ); - expect(toDataArray(snapshot2)).to.deep.equal([ - { key: 'd', sort: 2, v: 2 } - ]); + expect(toIds(snapshot2)).to.deep.equal(['doc4']); // With multiple IN const snapshot3 = await getDocs( @@ -1385,9 +1381,7 @@ apiDescribe('Queries', persistence => { where('sort', 'in', [2, 3]) ) ); - expect(toDataArray(snapshot3)).to.deep.equal([ - { key: 'd', sort: 2, v: 2 } - ]); + expect(toIds(snapshot3)).to.deep.equal(['doc4']); // With NOT-IN const snapshot4 = await getDocs( @@ -1398,10 +1392,7 @@ apiDescribe('Queries', persistence => { where('v', 'not-in', [2, 4, 5]) ) ); - expect(toDataArray(snapshot4)).to.deep.equal([ - { key: 'a', sort: 0, v: 0 }, - { key: 'c', sort: 1, v: 3 } - ]); + expect(toIds(snapshot4)).to.deep.equal(['doc1', 'doc3']); // With orderby const snapshot5 = await getDocs( @@ -1412,11 +1403,7 @@ apiDescribe('Queries', persistence => { orderBy('v', 'desc') ) ); - expect(toDataArray(snapshot5)).to.deep.equal([ - { key: 'c', sort: 1, v: 3 }, - { key: 'd', sort: 2, v: 2 }, - { key: 'a', sort: 0, v: 0 } - ]); + expect(toIds(snapshot5)).to.deep.equal(['doc3', 'doc4', 'doc1']); // With limit const snapshot6 = await getDocs( @@ -1428,10 +1415,7 @@ apiDescribe('Queries', persistence => { limit(2) ) ); - expect(toDataArray(snapshot6)).to.deep.equal([ - { key: 'c', sort: 1, v: 3 }, - { key: 'd', sort: 2, v: 2 } - ]); + expect(toIds(snapshot6)).to.deep.equal(['doc3', 'doc4']); // With limitToLast const snapshot7 = await getDocs( @@ -1443,10 +1427,7 @@ apiDescribe('Queries', persistence => { limitToLast(2) ) ); - expect(toDataArray(snapshot7)).to.deep.equal([ - { key: 'd', sort: 2, v: 2 }, - { key: 'a', sort: 0, v: 0 } - ]); + expect(toIds(snapshot7)).to.deep.equal(['doc4', 'doc1']); }); }); @@ -1463,10 +1444,7 @@ apiDescribe('Queries', persistence => { const snapshot1 = await getDocs( query(coll, where('key', '!=', 'a'), where('sort', '<=', 2)) ); - expect(toDataArray(snapshot1)).to.deep.equal([ - { key: 'e', sort: 1 }, - { key: 'f', sort: 1, v: 1 } - ]); + expect(toIds(snapshot1)).to.deep.equal(['doc5', 'doc6']); const snapshot2 = await getDocs( query( @@ -1476,9 +1454,7 @@ apiDescribe('Queries', persistence => { where('v', '<=', 1) ) ); - expect(toDataArray(snapshot2)).to.deep.equal([ - { key: 'f', sort: 1, v: 1 } - ]); + expect(toIds(snapshot2)).to.deep.equal(['doc6']); }); }); it('can use with array membership', async () => { @@ -1500,9 +1476,7 @@ apiDescribe('Queries', persistence => { where('v', 'array-contains', 0) ) ); - expect(toDataArray(snapshot1)).to.deep.equal([ - { key: 'b', sort: 1, v: [0, 1, 3] } - ]); + expect(toIds(snapshot1)).to.deep.equal(['doc2']); const snapshot2 = await getDocs( query( @@ -1512,10 +1486,7 @@ apiDescribe('Queries', persistence => { where('v', 'array-contains-any', [0, 1]) ) ); - expect(toDataArray(snapshot2)).to.deep.equal([ - { key: 'b', sort: 1, v: [0, 1, 3] }, - { key: 'd', sort: 2, v: [1] } - ]); + expect(toIds(snapshot2)).to.deep.equal(['doc2', 'doc4']); }); }); @@ -1535,10 +1506,10 @@ apiDescribe('Queries', persistence => { }; const testDocs = { - '1': testData(400), - '2': testData(200), - '3': testData(100), - '4': testData(300) + 'doc1': testData(400), + 'doc2': testData(200), + 'doc3': testData(100), + 'doc4': testData(300) }; return withTestCollection(persistence, testDocs, async coll => { @@ -1551,10 +1522,7 @@ apiDescribe('Queries', persistence => { orderBy('name') ) ); - expect(toDataArray(snapshot1)).to.deep.equal([ - testData(300), - testData(400) - ]); + expect(toIds(snapshot1)).to.deep.equal(['doc4', 'doc1']); const snapshot2 = await getDocs( query( @@ -1565,10 +1533,7 @@ apiDescribe('Queries', persistence => { orderBy('name', 'desc') ) ); - expect(toDataArray(snapshot2)).to.deep.equal([ - testData(200), - testData(100) - ]); + expect(toIds(snapshot2)).to.deep.equal(['doc2', 'doc3']); }); }); @@ -1592,11 +1557,11 @@ apiDescribe('Queries', persistence => { ) ); // Implicitly ordered by: 'key' asc, 'sort' asc, 'v' asc, __name__ asc - expect(toDataArray(snapshot1)).to.deep.equal([ - { key: 'a', sort: 0, v: 5 }, - { key: 'b', sort: 0, v: 0 }, - { key: 'b', sort: 2, v: 1 }, - { key: 'b', sort: 2, v: 2 } + expect(toIds(snapshot1)).to.deep.equal([ + 'doc1', + 'doc6', + 'doc5', + 'doc4' ]); const snapshot2 = await getDocs( @@ -1611,11 +1576,11 @@ apiDescribe('Queries', persistence => { ) ); // Ordered by: 'sort' desc, 'key' asc, 'v' asc, __name__ asc - expect(toDataArray(snapshot2)).to.deep.equal([ - { key: 'b', sort: 2, v: 1 }, - { key: 'b', sort: 2, v: 2 }, - { key: 'a', sort: 0, v: 5 }, - { key: 'b', sort: 0, v: 0 } + expect(toIds(snapshot2)).to.deep.equal([ + 'doc5', + 'doc4', + 'doc1', + 'doc6' ]); const snapshot3 = await getDocs( @@ -1624,7 +1589,7 @@ apiDescribe('Queries', persistence => { and( or( and(where('key', '==', 'b'), where('sort', '<=', 4)), - and(where('key', '!=', 'b'), where('v', '>', 4)) + and(where('key', '!=', 'b'), where('v', '>=', 4)) ), or( and(where('key', '>', 'b'), where('sort', '>=', 1)), @@ -1633,9 +1598,8 @@ apiDescribe('Queries', persistence => { ) ) ); - expect(toDataArray(snapshot3)).to.deep.equal([ - { key: 'a', sort: 0, v: 5 } - ]); + // Implicitly ordered by: 'key' asc, 'sort' asc, 'v' asc, __name__ asc + expect(toIds(snapshot3)).to.deep.equal(['doc1', 'doc2']); }); }); @@ -1658,11 +1622,11 @@ apiDescribe('Queries', persistence => { where('v', 'in', [1, 2, 3, 4]) ) ); - expect(toDataArray(snapshot1)).to.deep.equal([ - { key: 'aa', sort: 4, v: 4 }, - { key: 'b', sort: 2, v: 2 }, - { key: 'b', sort: 2, v: 1 }, - { key: 'b', sort: 3, v: 3 } + expect(toIds(snapshot1)).to.deep.equal([ + 'doc2', + 'doc4', + 'doc5', + 'doc3' ]); // Implicitly ordered by: 'key' asc, 'sort' asc,__name__ asc @@ -1674,11 +1638,11 @@ apiDescribe('Queries', persistence => { where('v', 'in', [1, 2, 3, 4]) ) ); - expect(toDataArray(snapshot2)).to.deep.equal([ - { key: 'aa', sort: 4, v: 4 }, - { key: 'b', sort: 2, v: 2 }, - { key: 'b', sort: 2, v: 1 }, - { key: 'b', sort: 3, v: 3 } + expect(toIds(snapshot2)).to.deep.equal([ + 'doc2', + 'doc4', + 'doc5', + 'doc3' ]); }); }); @@ -1703,11 +1667,11 @@ apiDescribe('Queries', persistence => { orderBy('v') ) ); - expect(toDataArray(snapshot1)).to.deep.equal([ - { key: 'aa', sort: 4, v: 0 }, - { key: 'b', sort: 2, v: 1 }, - { key: 'b', sort: 3, v: 1 }, - { key: 'bb', sort: 1, v: 1 } + expect(toIds(snapshot1)).to.deep.equal([ + 'doc2', + 'doc4', + 'doc3', + 'doc5' ]); // Ordered by: 'v asc, 'sort' asc, 'key' asc, __name__ asc @@ -1720,11 +1684,11 @@ apiDescribe('Queries', persistence => { orderBy('sort') ) ); - expect(toDataArray(snapshot2)).to.deep.equal([ - { key: 'aa', sort: 4, v: 0 }, - { key: 'bb', sort: 1, v: 1 }, - { key: 'b', sort: 2, v: 1 }, - { key: 'b', sort: 3, v: 1 } + expect(toIds(snapshot2)).to.deep.equal([ + 'doc2', + 'doc5', + 'doc4', + 'doc3' ]); // Implicit order by matches the direction of last explicit order by. @@ -1737,11 +1701,11 @@ apiDescribe('Queries', persistence => { orderBy('v', 'desc') ) ); - expect(toDataArray(snapshot3)).to.deep.equal([ - { key: 'bb', sort: 1, v: 1 }, - { key: 'b', sort: 3, v: 1 }, - { key: 'b', sort: 2, v: 1 }, - { key: 'aa', sort: 4, v: 0 } + expect(toIds(snapshot3)).to.deep.equal([ + 'doc5', + 'doc3', + 'doc4', + 'doc2' ]); // Ordered by: 'v desc, 'sort' asc, 'key' asc, __name__ asc @@ -1754,11 +1718,11 @@ apiDescribe('Queries', persistence => { orderBy('sort') ) ); - expect(toDataArray(snapshot4)).to.deep.equal([ - { key: 'bb', sort: 1, v: 1 }, - { key: 'b', sort: 2, v: 1 }, - { key: 'b', sort: 3, v: 1 }, - { key: 'aa', sort: 4, v: 0 } + expect(toIds(snapshot4)).to.deep.equal([ + 'doc5', + 'doc4', + 'doc3', + 'doc2' ]); }); }); @@ -1821,11 +1785,7 @@ apiDescribe('Queries', persistence => { ); // Document Key in inequality field will implicitly ordered to the last. // Implicitly ordered by: 'key' asc, 'sort' asc, __name__ asc - expect(toDataArray(snapshot1)).to.deep.equal([ - { key: 'aa', sort: 4 }, - { key: 'b', sort: 2 }, - { key: 'b', sort: 3 } - ]); + expect(toIds(snapshot1)).to.deep.equal(['doc2', 'doc4', 'doc3']); const snapshot2 = await getDocs( query( @@ -1837,11 +1797,7 @@ apiDescribe('Queries', persistence => { ); // Changing filters order will not effect implicit order. // Implicitly ordered by: 'key' asc, 'sort' asc, __name__ asc - expect(toDataArray(snapshot2)).to.deep.equal([ - { key: 'aa', sort: 4 }, - { key: 'b', sort: 2 }, - { key: 'b', sort: 3 } - ]); + expect(toIds(snapshot2)).to.deep.equal(['doc2', 'doc4', 'doc3']); const snapshot3 = await getDocs( query( @@ -1853,11 +1809,7 @@ apiDescribe('Queries', persistence => { ) ); // Ordered by: 'sort' desc,'key' desc, __name__ desc - expect(toDataArray(snapshot3)).to.deep.equal([ - { key: 'aa', sort: 4 }, - { key: 'b', sort: 3 }, - { key: 'b', sort: 2 } - ]); + expect(toIds(snapshot3)).to.deep.equal(['doc2', 'doc3', 'doc4']); }); }); @@ -1887,10 +1839,7 @@ apiDescribe('Queries', persistence => { expect(snapshot2.metadata.fromCache).to.be.true; expect(snapshot2.metadata.hasPendingWrites).to.be.false; // Implicitly ordered by: 'key' asc, 'sort' asc, __name__ asc - expect(toDataArray(snapshot2)).to.deep.equal([ - { key: 'b', sort: 2 }, - { key: 'b', sort: 3 } - ]); + expect(toIds(snapshot2)).to.deep.equal(['doc4', 'doc3']); } ); }); @@ -1921,6 +1870,7 @@ apiDescribe('Queries', persistence => { ); // explicit AND: a < 3 && b not-in [2, 3] + // Implicitly ordered by: a asc, __name__ asc await checkOnlineAndOfflineResultsMatch( query(coll, and(where('a', '<', 3), where('b', 'not-in', [2, 3]))), 'doc1', @@ -1930,7 +1880,7 @@ apiDescribe('Queries', persistence => { // a <3 && b != 0, implicitly ordered by: a asc, b asc, __name__ asc await checkOnlineAndOfflineResultsMatch( - query(coll, where('a', '<', 3), where('b', '!=', 0), limit(2)), + query(coll, where('b', '!=', 0), where('a', '<', 3), limit(2)), 'doc5', 'doc4' ); From 936b00a300aa69e05ed0ab7b41affee963d48995 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 20 Jul 2023 14:50:04 -0400 Subject: [PATCH 14/30] remove problematic use of andFilter, orFilter --- .../test/integration/api/validation.test.ts | 53 +++++++++++++++++++ .../firestore/test/unit/core/query.test.ts | 10 ---- 2 files changed, 53 insertions(+), 10 deletions(-) diff --git a/packages/firestore/test/integration/api/validation.test.ts b/packages/firestore/test/integration/api/validation.test.ts index 3e6c68683f1..e7f1a42c9f3 100644 --- a/packages/firestore/test/integration/api/validation.test.ts +++ b/packages/firestore/test/integration/api/validation.test.ts @@ -1314,6 +1314,59 @@ apiDescribe('Validation:', persistence => { ) ) ).not.to.throw(); + + // Composite queries can validate conflicting operators. + expect(() => + query( + coll, + and( + or( + and(where('a', '!=', 'b'), where('c', '>=', 'd')), + and(where('e', '==', 'f'), where('g', '!=', 'h')) + ), + or( + and(where('i', '==', 'j'), where('k', '>', 'l')), + and(where('m', '<=', 'n'), where('o', '<', 'p')) + ) + ) + ) + ).to.throw("Invalid query. You cannot use more than one '!=' filter."); + + expect(() => + query( + coll, + and( + or( + and(where('a', '==', 'b'), where('c', '>=', 'd')), + and(where('e', '==', 'f'), where('g', '!=', 'h')) + ), + or( + and(where('i', '==', 'j'), where('k', '>', 'l')), + and(where('m', '<=', 'n'), where('o', 'not-in', ['p'])) + ) + ) + ) + ).to.throw( + "Invalid query. You cannot use 'not-in' filters with '!=' filters." + ); + + expect(() => + query( + coll, + and( + or( + and(where('a', '==', 'b'), where('c', '>=', 'd')), + and(where('e', '==', 'f'), where('g', 'not-in', ['h'])) + ), + or( + and(where('i', '==', 'j'), where('k', '>', 'l')), + and(where('m', '<=', 'n'), where('o', 'not-in', ['p'])) + ) + ) + ) + ).to.throw( + "Invalid query. You cannot use more than one 'not-in' filter." + ); } ); }); diff --git a/packages/firestore/test/unit/core/query.test.ts b/packages/firestore/test/unit/core/query.test.ts index 49c17d70e4d..c0901e27f72 100644 --- a/packages/firestore/test/unit/core/query.test.ts +++ b/packages/firestore/test/unit/core/query.test.ts @@ -885,16 +885,6 @@ describe('Query', () => { ) ); assertQueryMatches(query3, [doc1, doc2, doc5], [doc3, doc4]); - - // (a!=1 || b!=1) && (a>=3 || b<3) - const query5 = query( - 'collection', - andFilter( - orFilter(filter('a', '!=', 1), filter('b', '!=', 1)), - orFilter(filter('a', '>=', 3), filter('b', '<', 3)) - ) - ); - assertQueryMatches(query5, [doc1, doc2, doc3], [doc4, doc5]); }); function assertQueryMatches( From 4e2262a8369f61618e403c5117050e8b1c50b0ed Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 21 Jul 2023 14:21:06 -0400 Subject: [PATCH 15/30] resolve comments --- packages/firestore/src/core/query.ts | 18 +++++++++------ .../src/model/target_index_matcher.ts | 22 ++++++++++++------- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index 3d3ee4f6aef..7c14f918d75 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -209,20 +209,20 @@ export function isCollectionGroupQuery(query: Query): boolean { } /** - * Returns the order by constraint that is used to execute the Query. Note that the implicit order by - * constraint can be different from the order by constraints the user provided (e.g. the SDK and + * Returns the order by constraint that is used to execute the Query. Note that the implicit order + * by constraint can be different from the order by constraints the user provided (e.g. the SDK and * backend always orders by `__name__`). */ export function queryOrderBy(query: Query): OrderBy[] { const queryImpl = debugCast(query, QueryImpl); if (queryImpl.memoizedOrderBy === null) { queryImpl.memoizedOrderBy = []; - const explicitFields = new Set(); + const fieldsNormalized = new Set(); // Any explicit order by fields should be added as is. for (const orderBy of queryImpl.explicitOrderBy) { queryImpl.memoizedOrderBy.push(orderBy); - explicitFields.add(orderBy.field.canonicalString()); + fieldsNormalized.add(orderBy.field.canonicalString()); } // The order of the implicit ordering always matches the last explicit order by. @@ -234,17 +234,21 @@ export function queryOrderBy(query: Query): OrderBy[] { // Any inequality fields not explicitly ordered should be implicitly ordered in a lexicographical // order. When there are multiple inequality filters on the same field, the field should be added // only once. - // Note: key field would be lexicographically ordered to the first by sorted set. + // Note: `SortedSet` sorts the key field before other fields. However, we want the key + // field to be sorted last. const inequalityFields: SortedSet = getInequalityFilterFields(queryImpl); inequalityFields.forEach(field => { - if (!explicitFields.has(field.canonicalString()) && !field.isKeyField()) { + if ( + !fieldsNormalized.has(field.canonicalString()) && + !field.isKeyField() + ) { queryImpl.memoizedOrderBy!.push(new OrderBy(field, lastDirection)); } }); // Add the document key field to the last if it is not explicitly ordered. - if (!explicitFields.has(FieldPath.keyField().canonicalString())) { + if (!fieldsNormalized.has(FieldPath.keyField().canonicalString())) { queryImpl.memoizedOrderBy.push( new OrderBy(FieldPath.keyField(), lastDirection) ); diff --git a/packages/firestore/src/model/target_index_matcher.ts b/packages/firestore/src/model/target_index_matcher.ts index 61752ea3cb2..292a1f63c50 100644 --- a/packages/firestore/src/model/target_index_matcher.ts +++ b/packages/firestore/src/model/target_index_matcher.ts @@ -19,6 +19,7 @@ import { FieldFilter, Operator } from '../core/filter'; import { Direction, OrderBy } from '../core/order_by'; import { Target } from '../core/target'; import { hardAssert } from '../util/assert'; +import { SortedSet } from '../util/sorted_set'; import { FieldIndex, @@ -27,6 +28,7 @@ import { IndexKind, IndexSegment } from './field_index'; +import { FieldPath } from './path'; /** * A light query planner for Firestore. @@ -52,7 +54,12 @@ export class TargetIndexMatcher { // The collection ID (or collection group) of the query target. private readonly collectionId: string; // The inequality filters of the target (if it exists). - private readonly inequalityFilters = new Map(); + // Note: The sort on FieldFilters is not required. Using SortedSet here just to utilize the custom + // comparator. + private inequalityFiltersSet = new SortedSet((lhs, rhs) => + FieldPath.comparator(lhs.field, rhs.field) + ); + // The list of equality filters of the target. private readonly equalityFilters: FieldFilter[]; // The list of orderBys of the target. @@ -68,10 +75,7 @@ export class TargetIndexMatcher { for (const filter of target.filters) { const fieldFilter = filter as FieldFilter; if (fieldFilter.isInequality()) { - this.inequalityFilters.set( - fieldFilter.field.canonicalString(), - fieldFilter - ); + this.inequalityFiltersSet = this.inequalityFiltersSet.add(fieldFilter); } else { this.equalityFilters.push(fieldFilter); } @@ -142,14 +146,16 @@ export class TargetIndexMatcher { return true; } - if (this.inequalityFilters.size > 0) { - if (this.inequalityFilters.size > 1) { + if (this.inequalityFiltersSet.size > 0) { + if (this.inequalityFiltersSet.size > 1) { // Only single inequality is supported for now. return false; } // Only a single inequality is currently supported. Get the only entry in the map. - const inequalityFilter = this.inequalityFilters.values().next().value; + const inequalityFilter = this.inequalityFiltersSet + .getIterator() + .getNext(); // 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. From 5909a08a80fbf5939f69be9a22f03548023c0946 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 21 Jul 2023 14:22:24 -0400 Subject: [PATCH 16/30] Update target_index_matcher.ts --- packages/firestore/src/model/target_index_matcher.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/firestore/src/model/target_index_matcher.ts b/packages/firestore/src/model/target_index_matcher.ts index 292a1f63c50..f82e03974f9 100644 --- a/packages/firestore/src/model/target_index_matcher.ts +++ b/packages/firestore/src/model/target_index_matcher.ts @@ -56,7 +56,7 @@ export class TargetIndexMatcher { // The inequality filters of the target (if it exists). // Note: The sort on FieldFilters is not required. Using SortedSet here just to utilize the custom // comparator. - private inequalityFiltersSet = new SortedSet((lhs, rhs) => + private inequalityFilters = new SortedSet((lhs, rhs) => FieldPath.comparator(lhs.field, rhs.field) ); @@ -75,7 +75,7 @@ export class TargetIndexMatcher { for (const filter of target.filters) { const fieldFilter = filter as FieldFilter; if (fieldFilter.isInequality()) { - this.inequalityFiltersSet = this.inequalityFiltersSet.add(fieldFilter); + this.inequalityFilters = this.inequalityFilters.add(fieldFilter); } else { this.equalityFilters.push(fieldFilter); } @@ -146,16 +146,14 @@ export class TargetIndexMatcher { return true; } - if (this.inequalityFiltersSet.size > 0) { - if (this.inequalityFiltersSet.size > 1) { + if (this.inequalityFilters.size > 0) { + if (this.inequalityFilters.size > 1) { // Only single inequality is supported for now. return false; } // Only a single inequality is currently supported. Get the only entry in the map. - const inequalityFilter = this.inequalityFiltersSet - .getIterator() - .getNext(); + const inequalityFilter = this.inequalityFilters.getIterator().getNext(); // 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. From 4fadda1ff9bad1646b9346e8a52b8b3687f8c563 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 25 Jul 2023 12:39:11 -0400 Subject: [PATCH 17/30] fix typo --- packages/firestore/test/integration/api/query.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 6d99a23605a..ce1fdc2eefe 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1886,7 +1886,7 @@ apiDescribe('Queries', persistence => { 'doc4' ); - // a <3 && b != 0, implicitly ordered by: b desc, a desc, __name__ desc + // a <3 && b != 0, ordered by: b desc, a desc, __name__ desc await checkOnlineAndOfflineResultsMatch( query( coll, @@ -1899,7 +1899,7 @@ apiDescribe('Queries', persistence => { 'doc2' ); - // explicit OR: multiple inequality: a>2 || b<=1. + // explicit OR: multiple inequality: a>2 || b<1. await checkOnlineAndOfflineResultsMatch( query(coll, or(where('a', '>', 2), where('b', '<', 1))), 'doc1', From 8ee0775cee5f12878e0a0efb04261034e1601c7d Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 25 Jul 2023 16:49:32 -0400 Subject: [PATCH 18/30] add changeset --- .changeset/gold-ways-yell.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/gold-ways-yell.md diff --git a/.changeset/gold-ways-yell.md b/.changeset/gold-ways-yell.md new file mode 100644 index 00000000000..5f1c6b12013 --- /dev/null +++ b/.changeset/gold-ways-yell.md @@ -0,0 +1,7 @@ +--- +'@firebase/firestore-compat': minor +'@firebase/firestore': minor +'firebase': minor +--- + +Support multiple inequality in compound queries. From 341d2023d88adec46d957960bc15be16934039f5 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 25 Jul 2023 16:58:32 -0400 Subject: [PATCH 19/30] resolve comment --- .changeset/gold-ways-yell.md | 2 +- packages/firestore/src/model/target_index_matcher.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.changeset/gold-ways-yell.md b/.changeset/gold-ways-yell.md index 5f1c6b12013..f1e7131a49f 100644 --- a/.changeset/gold-ways-yell.md +++ b/.changeset/gold-ways-yell.md @@ -4,4 +4,4 @@ 'firebase': minor --- -Support multiple inequality in compound queries. +Added support for multiple inequality in compound queries. diff --git a/packages/firestore/src/model/target_index_matcher.ts b/packages/firestore/src/model/target_index_matcher.ts index f82e03974f9..6708be069c1 100644 --- a/packages/firestore/src/model/target_index_matcher.ts +++ b/packages/firestore/src/model/target_index_matcher.ts @@ -152,7 +152,7 @@ export class TargetIndexMatcher { return false; } - // Only a single inequality is currently supported. Get the only entry in the map. + // Only a single inequality is currently supported. Get the only entry in the set. const inequalityFilter = this.inequalityFilters.getIterator().getNext(); // 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 From a586432a900747619f023f2d3b852f0fb79b1cd1 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 28 Jul 2023 10:40:56 -0400 Subject: [PATCH 20/30] change misnamed test case --- packages/firestore/test/integration/api/query.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index ce1fdc2eefe..8f4ff949692 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1432,7 +1432,7 @@ apiDescribe('Queries', persistence => { }); }); - it('can use on unary values', async () => { + it('can use on special values', async () => { const testDocs = { doc1: { key: 'a', sort: 0, v: 0 }, doc2: { key: 'b', sort: NaN, v: 1 }, From 59796bc579d8e0fcbce6e0acef560a0e856b3f47 Mon Sep 17 00:00:00 2001 From: Mila <107142260+milaGGL@users.noreply.github.com> Date: Tue, 8 Aug 2023 10:27:05 -0400 Subject: [PATCH 21/30] Simplify code and improve test coverage (#7512) --- packages/firestore/src/core/filter.ts | 17 --- packages/firestore/src/core/query.ts | 12 +- .../test/integration/api/validation.test.ts | 137 +----------------- .../firestore/test/unit/core/query.test.ts | 93 ++++++++++++ packages/firestore/test/util/helpers.ts | 2 +- 5 files changed, 101 insertions(+), 160 deletions(-) diff --git a/packages/firestore/src/core/filter.ts b/packages/firestore/src/core/filter.ts index adc187fef12..06e2740c315 100644 --- a/packages/firestore/src/core/filter.ts +++ b/packages/firestore/src/core/filter.ts @@ -56,8 +56,6 @@ export abstract class Filter { abstract getFlattenedFilters(): readonly FieldFilter[]; abstract getFilters(): Filter[]; - - abstract getInequalityFilters(): readonly FieldFilter[]; } export class FieldFilter extends Filter { @@ -194,13 +192,6 @@ export class FieldFilter extends Filter { getFilters(): Filter[] { return [this]; } - - getInequalityFilters(): readonly FieldFilter[] { - if (this.isInequality()) { - return [this]; - } - return []; - } } export class CompositeFilter extends Filter { @@ -246,14 +237,6 @@ export class CompositeFilter extends Filter { getFilters(): Filter[] { return Object.assign([], this.filters); } - - // Performs a depth-first search to find and return the inequality FieldFilters in the composite - // filter. Returns an empty array if none of the FieldFilters has inequality filters. - getInequalityFilters(): readonly FieldFilter[] { - return this.getFlattenedFilters().filter((filter: FieldFilter) => - filter.isInequality() - ); - } } export function compositeFilterIsConjunction( diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index 7c14f918d75..f3a9f2b46e6 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -26,7 +26,7 @@ import { boundSortsAfterDocument, boundSortsBeforeDocument } from './bound'; -import { Filter } from './filter'; +import { FieldFilter, Filter } from './filter'; import { Direction, OrderBy } from './order_by'; import { canonifyTarget, @@ -170,11 +170,11 @@ export function queryMatchesAllDocuments(query: Query): boolean { export function getInequalityFilterFields(query: Query): SortedSet { let result = new SortedSet(FieldPath.comparator); query.filters.forEach((filter: Filter) => { - const inequalityFields = filter - .getInequalityFilters() - .map(filter => filter.field); - inequalityFields.forEach((field: FieldPath) => { - result = result.add(field); + const subFilters = filter.getFlattenedFilters(); + subFilters.forEach((filter: FieldFilter) => { + if (filter.isInequality()) { + result = result.add(filter.field); + } }); }); return result; diff --git a/packages/firestore/test/integration/api/validation.test.ts b/packages/firestore/test/integration/api/validation.test.ts index e7f1a42c9f3..e43fb238c2a 100644 --- a/packages/firestore/test/integration/api/validation.test.ts +++ b/packages/firestore/test/integration/api/validation.test.ts @@ -1175,146 +1175,11 @@ apiDescribe('Validation:', persistence => { } ); - // Multiple Inequality validation tests - validationIt(persistence, 'can have multiple inequality fields', db => { - const coll = collection(db, 'test'); - expect(() => - query(coll, where('x', '>=', 32), where('y', '<', 42)) - ).not.to.throw(); - }); - - validationIt( - persistence, - 'can have != and inequality queries on different fields', - db => { - const coll = collection(db, 'test'); - expect(() => - query(coll, where('x', '>', 32), where('y', '!=', 42)) - ).not.to.throw(); - } - ); - validationIt( persistence, - 'can have not-in and inequality queries on different fields', + 'conflicting operators inside a nested composite filter', db => { const coll = collection(db, 'test'); - expect(() => - query(coll, where('y', '>=', 32), where('x', 'not-in', [42])) - ).not.to.throw(); - } - ); - - validationIt( - persistence, - 'can have inequality different than orderBy', - db => { - const coll = collection(db, 'test'); - // single inequality - expect(() => - query(coll, where('x', '>', 32), orderBy('y')) - ).not.to.throw(); - expect(() => - query(coll, orderBy('y'), where('x', '>', 32)) - ).not.to.throw(); - expect(() => - query(coll, where('x', '>', 32), orderBy('y'), orderBy('x')) - ).not.to.throw(); - expect(() => - query(coll, orderBy('y'), orderBy('x'), where('x', '>', 32)) - ).not.to.throw(); - expect(() => - query(coll, where('x', '!=', 32), orderBy('y')) - ).not.to.throw(); - - // multiple inequality - expect(() => - query(coll, where('x', '>', 32), where('y', '!=', 42), orderBy('z')) - ).not.to.throw(); - expect(() => - query(coll, orderBy('y'), where('x', '>', 32), where('y', '<=', 42)) - ).not.to.throw(); - expect(() => - query( - coll, - where('x', '>', 32), - where('y', '!=', 42), - orderBy('y'), - orderBy('x') - ) - ).not.to.throw(); - expect(() => - query( - coll, - orderBy('y'), - orderBy('z'), - where('x', '>', 32), - where('y', '!=', 42) - ) - ).not.to.throw(); - } - ); - - validationIt( - persistence, - 'multiple inequalities inside a nested composite filter', - db => { - // Multiple inequality on different fields inside a nested composite filter. - const coll = collection(db, 'test'); - expect(() => - query( - coll, - or( - and(where('a', '==', 'b'), where('c', '>', 'd')), - and(where('e', '<=', 'f'), where('g', '==', 'h')) - ) - ) - ).not.to.throw(); - - // Multiple inequality on different fields between a field filter and a composite filter. - expect(() => - query( - coll, - and( - or( - and(where('a', '==', 'b'), where('c', '>=', 'd')), - and(where('e', '==', 'f'), where('g', '!=', 'h')) - ), - where('r', '<', 's') - ) - ) - ).not.to.throw(); - - // OrderBy and multiple inequality on different fields. - expect(() => - query( - coll, - or( - and(where('a', '==', 'b'), where('c', '>', 'd')), - and(where('e', '==', 'f'), where('g', '!=', 'h')) - ), - orderBy('r'), - orderBy('a') - ) - ).not.to.throw(); - - // Multiple inequality inside two composite filters. - expect(() => - query( - coll, - and( - or( - and(where('a', '==', 'b'), where('c', '>=', 'd')), - and(where('e', '==', 'f'), where('g', '!=', 'h')) - ), - or( - and(where('i', '==', 'j'), where('k', '>', 'l')), - and(where('m', '==', 'n'), where('o', '<', 'p')) - ) - ) - ) - ).not.to.throw(); - // Composite queries can validate conflicting operators. expect(() => query( diff --git a/packages/firestore/test/unit/core/query.test.ts b/packages/firestore/test/unit/core/query.test.ts index c0901e27f72..c29f3fdda9b 100644 --- a/packages/firestore/test/unit/core/query.test.ts +++ b/packages/firestore/test/unit/core/query.test.ts @@ -777,6 +777,99 @@ describe('Query', () => { ); }); + it("generates the correct implicit order by's for multiple inequality", () => { + assertImplicitOrderBy( + query( + 'foo', + filter('a', '<', 5), + filter('aa', '>', 5), + filter('b', '>', 5), + filter('A', '>', 5) + ), + orderBy('A'), + orderBy('a'), + orderBy('aa'), + orderBy('b'), + orderBy(DOCUMENT_KEY_NAME) + ); + + // numbers + assertImplicitOrderBy( + query( + 'foo', + filter('a', '<', 5), + filter('1', '>', 5), + filter('19', '>', 5), + filter('2', '>', 5) + ), + orderBy('1'), + orderBy('19'), + orderBy('2'), + orderBy('a'), + + orderBy(DOCUMENT_KEY_NAME) + ); + + // nested fields + assertImplicitOrderBy( + query( + 'foo', + filter('a', '<', 5), + filter('aa', '>', 5), + filter('a.a', '>', 5) + ), + orderBy('a'), + orderBy('a.a'), + orderBy('aa'), + orderBy(DOCUMENT_KEY_NAME) + ); + + // special characters + assertImplicitOrderBy( + query( + 'foo', + filter('a', '<', 5), + filter('_a', '>', 5), + filter('a.a', '>', 5) + ), + orderBy('_a'), + orderBy('a'), + orderBy('a.a'), + orderBy(DOCUMENT_KEY_NAME) + ); + + // field name with dot + assertImplicitOrderBy( + query( + 'foo', + filter('a', '<', 5), + filter('a.z', '>', 5), // nested field + filter('`a.a`', '>', 5) // field name with dot + ), + orderBy('a'), + orderBy('a.z'), + orderBy('`a.a`'), + orderBy(DOCUMENT_KEY_NAME) + ); + + // composite filter + assertImplicitOrderBy( + query( + 'foo', + filter('a', '<', 5), + andFilter( + orFilter(filter('b', '>=', 1), filter('c', '<=', 1)), + orFilter(filter('d', '>', 1), filter('e', '==', 1)) + ) + ), + orderBy('a'), + orderBy('b'), + orderBy('c'), + orderBy('d'), + orderBy(DOCUMENT_KEY_NAME) + ); + }); + it('matchesAllDocuments() considers filters, orders and bounds', () => { const baseQuery = newQueryForPath(ResourcePath.fromString('collection')); expect(queryMatchesAllDocuments(baseQuery)).to.be.true; diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index 6cc59a5f779..344e4676777 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -227,7 +227,7 @@ export function path(path: string, offset?: number): ResourcePath { } export function field(path: string): FieldPath { - return new FieldPath(path.split('.')); + return FieldPath.fromServerFormat(path); } export function fieldIndex( From 0c0678e642ac9d4f25aa3ad02137d1f974115922 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 11 Aug 2023 11:14:36 -0400 Subject: [PATCH 22/30] format --- packages/firestore/src/core/query.ts | 4 +++- packages/firestore/test/unit/core/query.test.ts | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index dfa38141e45..b13296ad7ee 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -252,7 +252,9 @@ export function queryNormalizedOrderBy(query: Query): OrderBy[] { !fieldsNormalized.has(field.canonicalString()) && !field.isKeyField() ) { - queryImpl.memoizedNormalizedOrderBy!.push(new OrderBy(field, lastDirection)); + queryImpl.memoizedNormalizedOrderBy!.push( + new OrderBy(field, lastDirection) + ); } }); diff --git a/packages/firestore/test/unit/core/query.test.ts b/packages/firestore/test/unit/core/query.test.ts index dfdec2b2c1b..b196221abd1 100644 --- a/packages/firestore/test/unit/core/query.test.ts +++ b/packages/firestore/test/unit/core/query.test.ts @@ -979,7 +979,7 @@ describe('Query', () => { ) ); assertQueryMatches(query3, [doc1, doc2, doc5], [doc3, doc4]); - }) + }); it('generates appropriate order-bys for aggregate and non-aggregate targets', () => { const col = newQueryForPath(ResourcePath.fromString('collection')); From b26b2e9a199d5d237f9d18048fbf18cda9e25134 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 11 Aug 2023 11:16:07 -0400 Subject: [PATCH 23/30] update chengeset --- .changeset/gold-ways-yell.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/gold-ways-yell.md b/.changeset/gold-ways-yell.md index f1e7131a49f..4cab1b26b9d 100644 --- a/.changeset/gold-ways-yell.md +++ b/.changeset/gold-ways-yell.md @@ -4,4 +4,4 @@ 'firebase': minor --- -Added support for multiple inequality in compound queries. +Added support for having multiple inequality fields in compound queries. From 226a754a97a9a188a64cddd3197c93814a9f32cb Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 17 Aug 2023 12:28:45 -0400 Subject: [PATCH 24/30] add more unit tests for order by normalization --- .../firestore/test/unit/core/query.test.ts | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/packages/firestore/test/unit/core/query.test.ts b/packages/firestore/test/unit/core/query.test.ts index b196221abd1..fd0e6884c66 100644 --- a/packages/firestore/test/unit/core/query.test.ts +++ b/packages/firestore/test/unit/core/query.test.ts @@ -783,6 +783,7 @@ describe('Query', () => { query( 'foo', filter('a', '<', 5), + filter('a', '>=', 5), filter('aa', '>', 5), filter('b', '>', 5), filter('A', '>', 5) @@ -869,6 +870,50 @@ describe('Query', () => { orderBy('d'), orderBy(DOCUMENT_KEY_NAME) ); + + // OrderBy + assertImplicitOrderBy( + query( + 'foo', + filter('b', '<', 5), + filter('a', '>', 5), + filter('z', '>', 5), + orderBy('z') + ), + orderBy('z'), + orderBy('a'), + orderBy('b'), + orderBy(DOCUMENT_KEY_NAME) + ); + + // last explicit order by direction + assertImplicitOrderBy( + query( + 'foo', + filter('b', '<', 5), + filter('a', '>', 5), + orderBy('z', 'desc') + ), + orderBy('z', 'desc'), + orderBy('a', 'desc'), + orderBy('b', 'desc'), + orderBy(DOCUMENT_KEY_NAME, 'desc') + ); + + assertImplicitOrderBy( + query( + 'foo', + filter('b', '<', 5), + filter('a', '>', 5), + orderBy('z', 'desc'), + orderBy('c') + ), + orderBy('z', 'desc'), + orderBy('c'), + orderBy('a'), + orderBy('b'), + orderBy(DOCUMENT_KEY_NAME) + ); }); it('matchesAllDocuments() considers filters, orders and bounds', () => { From bceea26661220dbdd47d19ec6d4370d03fca4935 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Wed, 30 Aug 2023 16:48:35 -0400 Subject: [PATCH 25/30] CSI compatibility --- .../src/local/indexeddb_index_manager.ts | 8 +-- .../src/model/target_index_matcher.ts | 25 ++++++--- .../test/unit/local/local_store.test.ts | 53 +++++++++++++++++++ .../unit/model/target_index_matcher.test.ts | 26 +++++++-- 4 files changed, 98 insertions(+), 14 deletions(-) diff --git a/packages/firestore/src/local/indexeddb_index_manager.ts b/packages/firestore/src/local/indexeddb_index_manager.ts index 71c5c9a70bc..33da3faa41d 100644 --- a/packages/firestore/src/local/indexeddb_index_manager.ts +++ b/packages/firestore/src/local/indexeddb_index_manager.ts @@ -275,10 +275,10 @@ export class IndexedDbIndexManager implements IndexManager { return this.getIndexType(transaction, subTarget).next(type => { if (type === IndexType.NONE || type === IndexType.PARTIAL) { const targetIndexMatcher = new TargetIndexMatcher(subTarget); - return this.addFieldIndex( - transaction, - targetIndexMatcher.buildTargetIndex() - ); + const fieldIndex = targetIndexMatcher.buildTargetIndex(); + if (fieldIndex != null) { + return this.addFieldIndex(transaction, fieldIndex); + } } }); } diff --git a/packages/firestore/src/model/target_index_matcher.ts b/packages/firestore/src/model/target_index_matcher.ts index 879c50111cd..0a5797e2c60 100644 --- a/packages/firestore/src/model/target_index_matcher.ts +++ b/packages/firestore/src/model/target_index_matcher.ts @@ -83,6 +83,10 @@ export class TargetIndexMatcher { } } + hasMultipleInequality(): boolean { + return this.inequalityFilters.size > 1; + } + /** * Returns whether the index can be used to serve the TargetIndexMatcher's * target. @@ -110,6 +114,11 @@ export class TargetIndexMatcher { 'Collection IDs do not match' ); + if (this.hasMultipleInequality()) { + // Only single inequality is supported for now. + return false; + } + // If there is an array element, find a matching filter. const arraySegment = fieldIndexGetArraySegment(index); if ( @@ -148,11 +157,6 @@ export class TargetIndexMatcher { } if (this.inequalityFilters.size > 0) { - if (this.inequalityFilters.size > 1) { - // Only single inequality is supported for now. - return false; - } - // Only a single inequality is currently supported. Get the only entry in the set. const inequalityFilter = this.inequalityFilters.getIterator().getNext(); // If there is an inequality filter and the field was not in one of the @@ -187,8 +191,15 @@ export class TargetIndexMatcher { return true; } - /** Returns a full matched field index for this target. */ - buildTargetIndex(): FieldIndex { + /** + * Returns a full matched field index for this target. Currently multiple + * inequality query is not supported so function returns null. + */ + buildTargetIndex(): FieldIndex | null { + if (this.hasMultipleInequality()) { + return null; + } + // We want to make sure only one segment created for one field. For example, // in case like a == 3 and a > 2, Index {a ASCENDING} will only be created // once. diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index 845e73a6efb..d5ba12f4c91 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -3098,4 +3098,57 @@ function indexedDbLocalStoreTests( .toReturnChanged('coll/a') .finish(); }); + + it('index auto creation does not work with multiple inequality', () => { + const query_ = query( + 'coll', + filter('field1', '<', 5), + filter('field2', '<', 5) + ); + return ( + expectLocalStore() + .afterAllocatingQuery(query_) + .toReturnTargetId(2) + .afterIndexAutoCreationConfigure({ + isEnabled: true, + indexAutoCreationMinCollectionSize: 0, + relativeIndexReadCostPerDocument: 2 + }) + .afterRemoteEvents([ + docAddedRemoteEvent( + doc('coll/a', 10, { field1: 1, field2: 2 }), + [2], + [] + ), + docAddedRemoteEvent( + doc('coll/b', 10, { field1: 8, field2: 2 }), + [2], + [] + ), + docAddedRemoteEvent( + doc('coll/c', 10, { field1: 'string', field2: 2 }), + [2], + [] + ), + docAddedRemoteEvent(doc('coll/d', 10, { field1: 1 }), [2], []), + docAddedRemoteEvent( + doc('coll/e', 10, { field1: 4, field2: 4 }), + [2], + [] + ) + ]) + // First time query runs without indexes. + // Based on current heuristic, collection document counts (5) > + // 2 * resultSize (2). + // Full matched index will not be created since FieldIndex does not support multiple inequality. + .afterExecutingQuery(query_) + .toHaveRead({ documentsByKey: 0, documentsByCollection: 2 }) + .toReturnChanged('coll/a', 'coll/e') + .afterBackfillIndexes() + .afterExecutingQuery(query_) + .toHaveRead({ documentsByKey: 0, documentsByCollection: 2 }) + .toReturnChanged('coll/a', 'coll/e') + .finish() + ); + }); } diff --git a/packages/firestore/test/unit/model/target_index_matcher.test.ts b/packages/firestore/test/unit/model/target_index_matcher.test.ts index 8e295e17fef..3422ed862c0 100644 --- a/packages/firestore/test/unit/model/target_index_matcher.test.ts +++ b/packages/firestore/test/unit/model/target_index_matcher.test.ts @@ -25,7 +25,7 @@ import { newQueryForCollectionGroup } from '../../../src/core/query'; import { targetGetSegmentCount } from '../../../src/core/target'; -import { IndexKind } from '../../../src/model/field_index'; +import { FieldIndex, IndexKind } from '../../../src/model/field_index'; import { TargetIndexMatcher } from '../../../src/model/target_index_matcher'; import { fieldIndex, filter, orderBy, query } from '../../util/helpers'; @@ -956,12 +956,32 @@ describe('Target Bounds', () => { )); }); + describe('query with multiple inequality', () => { + it('returns null', () => { + const q = queryWithAddedFilter( + queryWithAddedFilter(query('collId'), filter('a', '>=', 1)), + filter('b', '<=', 10) + ); + const target = queryToTarget(q); + const targetIndexMatcher = new TargetIndexMatcher(target); + expect(targetIndexMatcher.hasMultipleInequality()).is.true; + const expectedIndex = targetIndexMatcher.buildTargetIndex(); + expect(expectedIndex).is.null; + }); + }); + function validateBuildTargetIndexCreateFullMatchIndex(q: Query): void { const target = queryToTarget(q); const targetIndexMatcher = new TargetIndexMatcher(target); + expect(targetIndexMatcher.hasMultipleInequality()).is.false; const expectedIndex = targetIndexMatcher.buildTargetIndex(); - expect(targetIndexMatcher.servedByIndex(expectedIndex)).is.true; - expect(expectedIndex.fields.length >= targetGetSegmentCount(target)); + expect(expectedIndex).is.not.null; + expect(targetIndexMatcher.servedByIndex(expectedIndex as FieldIndex)).is + .true; + expect( + (expectedIndex as FieldIndex).fields.length >= + targetGetSegmentCount(target) + ); } }); From 69947dc04d9234c8797ed08578416a3c4d19cace Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Thu, 31 Aug 2023 13:50:51 -0400 Subject: [PATCH 26/30] Address feedback --- packages/firestore/src/model/target_index_matcher.ts | 7 ++++--- .../test/unit/model/target_index_matcher.test.ts | 12 ++++++------ 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/firestore/src/model/target_index_matcher.ts b/packages/firestore/src/model/target_index_matcher.ts index 0a5797e2c60..df80ffa419a 100644 --- a/packages/firestore/src/model/target_index_matcher.ts +++ b/packages/firestore/src/model/target_index_matcher.ts @@ -83,7 +83,7 @@ export class TargetIndexMatcher { } } - hasMultipleInequality(): boolean { + get hasMultipleInequality(): boolean { return this.inequalityFilters.size > 1; } @@ -114,8 +114,9 @@ export class TargetIndexMatcher { 'Collection IDs do not match' ); - if (this.hasMultipleInequality()) { + if (this.hasMultipleInequality) { // Only single inequality is supported for now. + // TODO(Add support for multiple inequality query): b/298441043 return false; } @@ -196,7 +197,7 @@ export class TargetIndexMatcher { * inequality query is not supported so function returns null. */ buildTargetIndex(): FieldIndex | null { - if (this.hasMultipleInequality()) { + if (this.hasMultipleInequality) { return null; } diff --git a/packages/firestore/test/unit/model/target_index_matcher.test.ts b/packages/firestore/test/unit/model/target_index_matcher.test.ts index 3422ed862c0..3b0189b892a 100644 --- a/packages/firestore/test/unit/model/target_index_matcher.test.ts +++ b/packages/firestore/test/unit/model/target_index_matcher.test.ts @@ -964,7 +964,7 @@ describe('Target Bounds', () => { ); const target = queryToTarget(q); const targetIndexMatcher = new TargetIndexMatcher(target); - expect(targetIndexMatcher.hasMultipleInequality()).is.true; + expect(targetIndexMatcher.hasMultipleInequality).is.true; const expectedIndex = targetIndexMatcher.buildTargetIndex(); expect(expectedIndex).is.null; }); @@ -973,13 +973,13 @@ describe('Target Bounds', () => { function validateBuildTargetIndexCreateFullMatchIndex(q: Query): void { const target = queryToTarget(q); const targetIndexMatcher = new TargetIndexMatcher(target); - expect(targetIndexMatcher.hasMultipleInequality()).is.false; - const expectedIndex = targetIndexMatcher.buildTargetIndex(); - expect(expectedIndex).is.not.null; - expect(targetIndexMatcher.servedByIndex(expectedIndex as FieldIndex)).is + expect(targetIndexMatcher.hasMultipleInequality).is.false; + const actualIndex = targetIndexMatcher.buildTargetIndex(); + expect(actualIndex).is.not.null; + expect(targetIndexMatcher.servedByIndex(actualIndex as FieldIndex)).is .true; expect( - (expectedIndex as FieldIndex).fields.length >= + (actualIndex as FieldIndex).fields.length >= targetGetSegmentCount(target) ); } From feea4ad699776746931e0bb48d3f1e186ea2bcb0 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Thu, 31 Aug 2023 13:57:19 -0400 Subject: [PATCH 27/30] Address feedback2 --- .../firestore/test/unit/model/target_index_matcher.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/firestore/test/unit/model/target_index_matcher.test.ts b/packages/firestore/test/unit/model/target_index_matcher.test.ts index 3b0189b892a..ed00cfa3145 100644 --- a/packages/firestore/test/unit/model/target_index_matcher.test.ts +++ b/packages/firestore/test/unit/model/target_index_matcher.test.ts @@ -965,8 +965,8 @@ describe('Target Bounds', () => { const target = queryToTarget(q); const targetIndexMatcher = new TargetIndexMatcher(target); expect(targetIndexMatcher.hasMultipleInequality).is.true; - const expectedIndex = targetIndexMatcher.buildTargetIndex(); - expect(expectedIndex).is.null; + const actualIndex = targetIndexMatcher.buildTargetIndex(); + expect(actualIndex).is.null; }); }); From 0d3c4525ebad66ba42b15942c0c7098f3c733197 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 7 Sep 2023 13:43:37 -0400 Subject: [PATCH 28/30] update test cooment --- packages/firestore/test/integration/api/query.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 5b73819defb..4982b7bcc21 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1877,7 +1877,7 @@ apiDescribe('Queries', persistence => { ); // explicit AND: a < 3 && b not-in [2, 3] - // Implicitly ordered by: a asc, __name__ asc + // Implicitly ordered by: a asc, b asc, __name__ asc await checkOnlineAndOfflineResultsMatch( query(coll, and(where('a', '<', 3), where('b', 'not-in', [2, 3]))), 'doc1', From 31c2133aac739edb922cbe40c81d2100060ce5fc Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 12 Sep 2023 15:12:11 -0400 Subject: [PATCH 29/30] Update local_store_indexeddb.test.ts --- .../unit/local/local_store_indexeddb.test.ts | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/packages/firestore/test/unit/local/local_store_indexeddb.test.ts b/packages/firestore/test/unit/local/local_store_indexeddb.test.ts index 8da12ded95d..e88b8d6a978 100644 --- a/packages/firestore/test/unit/local/local_store_indexeddb.test.ts +++ b/packages/firestore/test/unit/local/local_store_indexeddb.test.ts @@ -892,4 +892,49 @@ describe('LocalStore w/ IndexedDB Persistence (Non generic)', () => { test.assertRemoteDocumentsRead(0, 1); test.assertQueryReturned('coll/a'); }); + + it('index auto creation does not work with multiple inequality', async () => { + const query_ = query( + 'coll', + filter('field1', '<', 5), + filter('field2', '<', 5) + ); + + const targetId = await test.allocateQuery(query_); + test.configureIndexAutoCreation({ + isEnabled: true, + indexAutoCreationMinCollectionSize: 0, + relativeIndexReadCostPerDocument: 2 + }); + + await test.applyRemoteEvents( + docAddedRemoteEvent(doc('coll/a', 10, { field1: 1, field2: 2 }), [ + targetId + ]), + docAddedRemoteEvent(doc('coll/b', 10, { field1: 8, field2: 2 }), [ + targetId + ]), + docAddedRemoteEvent(doc('coll/c', 10, { field1: 'string', field2: 2 }), [ + targetId + ]), + docAddedRemoteEvent(doc('coll/d', 10, { field1: 1 }), [targetId]), + docAddedRemoteEvent(doc('coll/e', 10, { field1: 4, field2: 4 }), [ + targetId + ]) + ); + + // First time query runs without indexes. + // Based on current heuristic, collection document counts (5) > + // 2 * resultSize (2). + // Full matched index will not be created since FieldIndex does not + // support multiple inequality. + await test.executeQuery(query_); + test.assertRemoteDocumentsRead(0, 2); + test.assertQueryReturned('coll/a', 'coll/e'); + + await test.backfillIndexes(); + await test.executeQuery(query_); + test.assertRemoteDocumentsRead(0, 2); + test.assertQueryReturned('coll/a', 'coll/e'); + }); }); From 4c154aac6d2f066ff62540037f726c93566b2304 Mon Sep 17 00:00:00 2001 From: Mila <107142260+milaGGL@users.noreply.github.com> Date: Tue, 12 Sep 2023 15:54:23 -0400 Subject: [PATCH 30/30] Delete changelog --- .changeset/gold-ways-yell.md | 7 ------- 1 file changed, 7 deletions(-) delete mode 100644 .changeset/gold-ways-yell.md diff --git a/.changeset/gold-ways-yell.md b/.changeset/gold-ways-yell.md deleted file mode 100644 index 4cab1b26b9d..00000000000 --- a/.changeset/gold-ways-yell.md +++ /dev/null @@ -1,7 +0,0 @@ ---- -'@firebase/firestore-compat': minor -'@firebase/firestore': minor -'firebase': minor ---- - -Added support for having multiple inequality fields in compound queries.