Derive pagination LIMIT/OFFSET from a SQL parser#286
Open
debba wants to merge 3 commits into
Open
Conversation
The grid pagination rewriter read the user's LIMIT/OFFSET with a hand-rolled token scanner that only recognised the trailing `LIMIT <n> OFFSET <n>` shape. It mis-handled MySQL's `LIMIT <offset>, <count>`, `OFFSET` before `LIMIT`, and numeric expressions, producing wrong values or a malformed appended query. Parse the query with sqlparser using the driver's dialect and read LIMIT/OFFSET from the AST instead, normalising the MySQL comma form. The trailing clause is stripped at its keyword (consistent with what the parser saw) and the new pagination clause is rendered from a LimitClause AST node, then concatenated to the verbatim sliced base so leading comments, inline hints, and formatting are preserved. The token scanner is kept as a fallback for inputs the parser rejects, so behaviour never regresses. FETCH FIRST ... ROWS is out of scope and defers to the fallback. Builds on #275.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (21 files)
Reviewed by kimi-k2.6-20260420 · 894,987 tokens |
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.
Builds on #275.
Background
PR #275 fixed the grid pagination rewriter to fold the user's
OFFSETinto theper-page offset, but it did so by extending the hand-rolled token scanner in
src-tauri/src/drivers/common/query.rs, which only recognises the trailing… LIMIT <n> OFFSET <n>shape.That heuristic is fragile. It does not understand:
LIMIT <offset>, <count>syntaxOFFSETbeforeLIMIT(valid in Postgres)In those cases the values are read wrong (or dropped) and the stripped base can be
inconsistent with what was extracted, producing a malformed appended query.
The change
sqlparsercrate (v0.62) and readQuery.limit_clausefrom the AST,parsed with the correct per-driver dialect (
PaginationDialect::{MySql,Postgres,Sqlite}threaded through
build_paginated_query). MySQL'sLIMIT <offset>, <count>isnormalised to the same
(limit, offset)shape.LIMIT/OFFSETkeyword (reusing the existingposition-aware tokenizer, which collapses parenthesised subqueries so inner
LIMITs are never touched), consistent with what the parser saw.LimitClauseAST node and concatenate itto the verbatim sliced base, so leading comments, inline
/*+ hints */, andthe body's formatting are preserved (no full-query reserialization).
so behaviour never regresses.
FETCH FIRST … ROWSis out of scope and defers tothe fallback.
Tests
Existing
build_paginated_querytests updated to pass a dialect; new cases added forthe MySQL comma form (pages 1 & 2),
OFFSETbeforeLIMIT, backtick identifiers,inline-hint preservation, and the parse-error fallback.
cargo test drivers::common— 63 passed.