Repository navigation
fix(query-generator): resolve malformed SQL and cursor pagination across List RPCs - #983
Merged
Merged
Conversation
…oss List RPCs
Fix server-side Spanner SQL syntax errors ("Syntax error: Unexpected keyword ORDER"
from malformed "WHERE ORDER BY") and cursor pagination defects across all List*
endpoints:
1. Extract validateAndOrderPageTokenFields in internal/query_generator/querytools
to filter page-token cursor fields against non-constant pageTokenFields
(excluding equalityFilters) and return InvalidArgument whenever any required
ORDER BY cursor key is missing or nil.
2. Parse RFC3339 string timestamps back into time.Time in DecodeAfterCursorPageToken
so Spanner parameter bindings for TIMESTAMP columns (created, updated, completed)
preserve type fidelity after JSON round-tripping.
3. Add bit_reversed_xid to Experiments, Artifacts, and StatusMessages query schema
FieldToTableMappings and include bit_reversed_xid in cursor extractors across
Experiments, XidEntries, WorkUnits, Artifacts, and StatusMessages.
4. Normalize page_size == 0 to DefaultPageSize (50) in ListXidEntries and reject
whitespace-only filter strings in ValidateFilter.
Fixes bug 564895895
TAG=agy
CONV=3a77ad46-f4de-418d-a279-ab38b9667efd
There was a problem hiding this comment.
Code Review
This pull request enhances the Spanner query generator and cursor-based pagination to support nullable columns, ensuring correct SQL generation (such as using IS NULL and IS NOT NULL predicates) and preserving int64 types during page token decoding. It also integrates bit_reversed_xid into various cursor extraction helpers and increases the maximum page token length. Feedback on these changes highlights a potential issue with hardcoding "work_unit_id": nil in ExtractStatusMessageCursor which could break pagination for status messages belonging to a work unit, as well as a mismatch between the documentation and implementation of nextPageTokenPrefix regarding nullability checks.
… prefix helper Comment link: #983 (comment) Comment link: #983 (comment) TAG=agy CONV=3a77ad46-f4de-418d-a279-ab38b9667efd
pachopepe
marked this pull request as ready for review
September 25, 2026 09:57
AntonTyb
previously approved these changes
Sep 25, 2026
pachopepe
enabled auto-merge
September 25, 2026 15:31
pachopepe
disabled auto-merge
September 25, 2026 15:31
andrzejfudro
previously approved these changes
Sep 28, 2026
…ation - Document rationale for 1024-character maxPageTokenLen in internal/validator/parameters.go - Remove redundant xid-to-bit_reversed_xid derivation from normalizeCursorNumbers in internal/query_generator/liststatement - Return InvalidArgument in resolveOrderedPageTokenFields when any required sort field is missing from ptFields - Set OrderByFieldsWithSortDirection on DashboardChartsWithCharts to order and tie-break by chart_id Comment link: #983 (comment) Comment link: #983 (comment) Comment link: #983 (comment) TAG=agy CONV=24574bbf-71a0-47f1-8939-622fd31d7bbe
…to fix/list-page-with-page-size
…into fix/list-page-with-page-size
pachopepe
dismissed stale reviews from andrzejfudro and AntonTyb
via
September 28, 2026 12:12
86c4e1e
andrzejfudro
approved these changes
Sep 28, 2026
pachopepe
enabled auto-merge
September 28, 2026 13:17
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Fixes server-side Spanner SQL syntax errors (
Syntax error: Unexpected keyword ORDERcaused by emittingWHERE ORDER BY) and cursor pagination defects when listing resources withpage_size,page_token, and optionalfilterororder_byparameters.Motivation & Context
When paginating
ListExperiments(orListXidEntries,ListWorkUnits,ListArtifacts,ListStatusMessages) withpage_sizeand nofilter,GenerateQuerydefaultedORDER BYto primary key columns (such asbit_reversed_xid), while cursor extractors only populated user-facing fields (such asxid). Consequently,orderedPtFieldsevaluated to empty whilelen(ptFields) > 0triggered theWHEREkeyword emission, producing invalid SQL (SELECT ... FROM Experiments e WHERE ORDER BY e.BitReversedXid LIMIT @limit). Additionally, JSON serialization oftime.Timecursor values inEncodeAfterCursorPageTokendecoded back asstringinDecodeAfterCursorPageToken, causing SpannerTIMESTAMPparameter type mismatches when ordering bycreated,updated, orcompleted.Key Changes
internal/query_generator/querytools): ExtractedresolveOrderedPageTokenFieldsto match cursor fields againstpageTokenFields(respectingequalityFiltersfrombaseFilter), guardWHEREemission onlen(orderedPtFields) > 0, and returnInvalidArgumentwhen any required sort field is missing fromptFields.internal/query_generator/liststatement): UpdatedDecodeAfterCursorPageTokento parse numeric cursor values viajson.Numberand coerce RFC3339 timestamp strings back intotime.Timefor SpannerTIMESTAMPbind parameters.internal/storage/spanner/...): Addedbit_reversed_xidmappings toexperimentschema,artifactschema, andstatusmessageschema, populatedbit_reversed_xid(spannerutils.ReverseBits(xid)) inExtractExperimentCursor,ExtractXidCursor,ExtractWorkUnitCursor,ExtractArtifactCursor, andExtractStatusMessageCursor, and configuredOrderByFieldsWithSortDirectiononDashboardChartsWithChartsto order and tie-break bychart_id.internal/validator,cmd/xid_service/server): AddednormalizePageSizeinListXidEntries(page_size == 0defaults todefaultXidPageSize = 10, matchingExperimentStateServer), documented the1024-charactermaxPageTokenLenlimit inValidatePageToken, and rejected whitespace-only filters inValidateFilter.Assumptions
Verification & Testing
bazel test //internal/query_generator/... //internal/validator/... //internal/storage/spanner/... //cmd/experiment_state_server/... //cmd/xid_service/...&bazel coverage --combined_report=lcov //internal/query_generator/... //internal/validator/...ListExperiments,ListXidEntries,ListWorkUnits, andListArtifacts),bazel coveragepasses across all 6 query generator and validator targets,.githooks/pre-commitpasses with 0 findings, and end-to-end pagination was verified on a live GKE development environment.Related Issues / Bugs
Fixes bug 564895895
TAG=agy
CONV=3a77ad46-f4de-418d-a279-ab38b9667efd