Move sum&avg composite index tests to composite_index_query.test.ts #7713
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
milaGGL
commented
Oct 20, 2023
•
milaGGL
commented
- update terraform to accept collection group index and array membership query
- add new composite indexes required for testing
- move sum&avg composite index tests to composite_index_query.test.ts
|
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
| }); | ||
| }); | ||
|
|
||
| it('performs aggregations on documents with all aggregated fields using getAggregationFromServer', () => { |
There was a problem hiding this comment.
The tests below and all new supporting indexes look good.
For your consideration, we could also change field names in these tests to re-use existing indexes instead of creating three new indexes. In the long run this practice would reduce the size of our TF files. Perhaps also extend the amount of time before we hit index limits: https://firebase.google.com/docs/firestore/quotas#indexes
There was a problem hiding this comment.
Actually, I see that you even mention this in your notes at the top of this file. Do you want my help making these changes since they are aggregate tests?
There was a problem hiding this comment.
I have changed the field name from "x" to "a", so no new fields introduced.
But since this is a CollectionGroup query, a new index is required after all. As long as any new tests on collection group query will keep using field "a", then we have achieved the re-use and limit index numbers.
There was a problem hiding this comment.
I was thinking about the non-collection-group indexes. But after comparing the new indexes to the existing ones, what you said still applies, we'd still have to create new indexes even if we changed field names.
| `abc/123/zzz${collectionGroupId}/cg-doc4`, | ||
| `abc/123/zzz/${collectionGroupId}` | ||
| ]; | ||
| const batch = writeBatch(db); |
There was a problem hiding this comment.
Does writeBatch work with the test helper library you created for composite index queries? If not, perhaps we should migrate this code over to use the addDoc(...) method that will add TTL fields and more?
There was a problem hiding this comment.
I used testHelper.addTestSpecificFieldsToDoc to add the TTL and testID fields, with minimal change to the existing code:
batch.set(
doc(db, docPath),
testHelper.addTestSpecificFieldsToDoc({ a: 2 }) // original: { a: 2 }
);
If there is no preference between write Batch and addDoc(), I can refactor the code. @MarkDuckworth
There was a problem hiding this comment.
Right. I see what you did now. Thanks. I don't have a strong preference between batch and adding single docs.
MarkDuckworth
approved these changes
Oct 25, 2023
| `abc/123/zzz${collectionGroupId}/cg-doc4`, | ||
| `abc/123/zzz/${collectionGroupId}` | ||
| ]; | ||
| const batch = writeBatch(db); |
There was a problem hiding this comment.
Right. I see what you did now. Thanks. I don't have a strong preference between batch and adding single docs.
| }); | ||
| }); | ||
|
|
||
| it('performs aggregations on documents with all aggregated fields using getAggregationFromServer', () => { |
There was a problem hiding this comment.
I was thinking about the non-collection-group indexes. But after comparing the new indexes to the existing ones, what you said still applies, we'd still have to create new indexes even if we changed field names.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.