Repository navigation
Conversation
Pipeline.create(from:) converts query cursors to where stages with WhereConditionsFromCursor, which builds one lexicographic disjunct per cursor value. For inclusive bounds the comparison was relaxed in the disjunct at index orderings.size() - 1 instead of the last disjunct, so the result was only correct when the cursor had one value per explicit orderBy. Document snapshot cursors ignored their document key component and returned documents that tie on the ordered fields but are on the wrong side of the cursor document, while key-only and partial cursors dropped the boundary documents. Make the comparison on the last cursor component inclusive, matching the JS and Android SDKs, and add tests that run converted queries and compare the results with Query::Matches. Fixes #16795
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where pipelines created from a query with an inclusive cursor returned incorrect documents at the cursor boundary by updating the boundary condition in WhereConditionsFromCursor to compare against cursors.size() instead of orderings.size() - 1. Comprehensive unit tests were also added to verify the fix. The review feedback recommends adding a defensive HARD_ASSERT to ensure cursors.size() <= orderings.size(), preventing potential out-of-bounds vector access.
…conversion WhereConditionsFromCursor indexes the orderings by cursor position, so a cursor with more values than orderings would read past the end of the vector. Assert the invariant, as Bound::CompareToDocument does, and add a test. The API layer already rejects such cursors, as suggested in review.
Problem
db.pipeline().create(from: query)turns query cursors intowherestages withWhereConditionsFromCursor. The function builds the usual lexicographicpredicate,
(f0 > v0) OR (f0 == v0 AND f1 > v1) OR ..., with one disjunct percursor value. For inclusive bounds (
start(at:),end(at:),start(atDocument:),end(atDocument:)), it was supposed to relax thecomparison in the last disjunct. Instead it relaxed the disjunct at index
orderings.size() - 1. That disjunct is the last one only when the cursor hasexactly one value fewer than the normalized orderings, i.e. one value per
explicit
orderBy. In all other cases the pipeline returned the wrongdocuments:
including
__name__. The second-to-last disjunct became inclusive, so the keycomponent was ignored. Documents that tie with the cursor document on the
ordered fields but sort before it (for
start) or after it (forend) werereturned.
orderBy, a key-only cursor never got the inclusivecomparison, so the boundary document was dropped.
orderBys), the inclusivecomparison was never applied either.
start(at:)behaved likestart(after:)andend(at:)likeend(before:).The JS (
whereConditionsFromCursorinpipeline-util.ts) and Android(
Query.whereConditionsFromCursor) SDKs apply the inclusive comparison to thelast cursor component.
Change
sub_end == cursors.size()), matching JS and Android.PipelineUtilTestcases that convert queries with cursors viaToPipelineStages, run the pipeline locally withRunPipeline, and check theresults. The cases cover document key cursors, key-only cursors, and partial
cursors, plus a matrix of cursor lengths x inclusive/exclusive x
start/endthat compares the results withQuery::Matches.Note: the Swift integration test
QueryToPipelineTests.testSupportsStartAtWithDocumentSnapshotis skipped("sort on name is not working"). It covers this case, and with the old code
its second half also returns document "2". Its JS twin runs unskipped. It can
probably be re-enabled in a follow-up once verified against the Enterprise
backend. This PR does not change it.
Testing
(Run locally with
-derivedDataPath <dir> -jobs 4added.)pipeline_util.ccchange reverted): the 4 new testsfail with 13 assertion failures, and the 6 existing
PipelineUtilTesttestspass.
ToPipelineStagesInclusiveBoundsWithDocumentKeyCursor: starting at[2, k/3]returns k/2..k/5 (expected k/3..k/5). Ending at[2, k/3]returns k/1..k/4 (expected k/1..k/3).
ToPipelineStagesInclusiveBoundsWithPartialCursor: starting at[2](ordered by
a, b) returns only k/5 (expected k/2..k/5). Ending at[2]returns only k/1 (expected k/1..k/4).
ToPipelineStagesInclusiveBoundsOnDocumentKeyOnly: starting at[k/3]returns k/4, k/5. Ending at
[k/3]returns k/1, k/2.ToPipelineStagesCursorsMatchQuerySemantics: 7 mismatches againstQuery::Matches.PipelineUtilTestTestspasses 10/10. The fullFirestore_Tests_macOStarget passes (3120 tests, 0 failures)../scripts/style.shon the changed files andgit diff --checkare clean.Fixes #16795