Skip to content

fix(datastore): honor the cursor when a filter fixes every sort column - #3450

Open
amanfredi wants to merge 1 commit into
authzed:mainfrom
amanfredi:fix-fully-filtered-cursor
Open

amanfredi wants to merge 1 commit into
authzed:mainfrom
amanfredi:fix-fully-filtered-cursor

Conversation

@amanfredi

Copy link
Copy Markdown

Description

Fixes #3449. A ReadRelationships filter that fixes all six relationship fields, resumed with a cursor, returned the cursor's own relationship on every page instead of an empty page.

CursorComparisonExpr leaves out columns the query already pins to one value. When all six are pinned, it returned nil and After added no predicate. Now, when every column is static, it compares all of them, which is the literal "sorts after the cursor" condition. Comparing all of them is still correct if the cursor's values differ from the filter's. It only applies when the filter matches at most one row, so the performance reason for omitting static columns doesn't apply. Partially pinned filters generate exactly the same SQL as before.

The CockroachDB cursored-delete path is the other caller. Its resumed batch now carries the full-key comparison rather than no predicate. That still matches nothing once the row is deleted. This mirrors the fallback cursorOrderColumns already uses for its ORDER BY.

BeforeOrEqual has a similar guard, which I left alone. With every column pinned it emits no predicate, so it returns the single matching row. That is correct when the cursor's values equal the filter's, which is the same assumption the existing static-column elision already makes. It can be wrong for a cursor taken from a different query. Its only caller, partitioned export, passes an empty filter, so it can't reach that case today. The same fallback would close it if you'd prefer the symmetry.

Changes

  • CursorComparisonExpr falls back to comparing every cursor column when all of them are static, and no longer returns nil.
  • After and the CockroachDB cursored delete drop their nil checks. The cursorOrderColumns comment and the fully pinned delete test now describe and assert the full-key predicate.
  • New datastore conformance test TestFullyFilteredCursor: for one relationship with an ellipsis subject relation and one without, sorted both ways, it pages with limit 1 until it gets an empty page and expects exactly one relationship.
  • New TestSchemaQueryFilterer case for a fully pinned filter in both comparison forms. The existing all-static CursorComparisonExpr unit test now asserts the full comparison instead of nil.
  • CHANGELOG entry under Unreleased.

Testing

TestFullyFilteredCursor on main without the fix fails in every case against Postgres 18, CockroachDB v26.2.5, MySQL 8.4 and the Spanner emulator, returning the relationship on all 5 pages. With the fix it passes on all four and on memdb. The new TestSchemaQueryFilterer case and the updated CursorComparisonExpr test also fail without the fix and pass with it.

With the fix, the full datastore suites pass on Postgres, MySQL and Spanner. On CockroachDB, the full suite passes, including the cursored-delete tests with and without integrity, apart from TestRetries, which also fails intermittently on main without this change. The unit tests in internal/datastore/common and internal/services/v1 pass.

🤖 Generated with Claude Code

CursorComparisonExpr omits columns that the query already pins to a single
value. When a ReadRelationships filter fixes all six relationship fields,
that left no columns, so it returned no predicate and the resumed query
returned the cursor's own row on every page, never an empty one. When every
column is static, compare all of them instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@amanfredi
amanfredi requested a review from a team as a code owner November 2, 2026 23:42
@github-actions github-actions Bot added area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Nov 2, 2026
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@amanfredi

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

authzedbot added a commit to authzed/cla that referenced this pull request Nov 2, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ReadRelationships repeats the same relationship on every cursor page when the filter fixes all six relationship fields

1 participant