Skip to content

fix(limiter): place the bound before the dialect's trailing clauses (#1398) - #1501

Merged
cevheri merged 2 commits into
libredb:mainfrom
KodYazicam:fix/1398-limiter-trailing-clauses
Oct 6, 2026
Merged

cevheri merged 2 commits into
libredb:mainfrom
KodYazicam:fix/1398-limiter-trailing-clauses

Conversation

@KodYazicam

Copy link
Copy Markdown
Contributor

Description

Closes #1398.

Two defects from one shape: the limiter appended the bound at the end of the statement, and some dialects have a clause that is written after the row bound.

  • With the clause present, every bounded read was refused outright: Materialize Expected end of statement, found LIMIT, ScyllaDB line 1:38 : Syntax error for BYPASS CACHE and line 1:42 for USING TIMEOUT 5s.
  • With the user's own bound present (LIMIT 10 BYPASS CACHE), the end-anchored probes could not see it - the bound was not the last thing in the statement - so a second bound was appended and the engine refused the pair. The Cassandra path had the worse version of this: … LIMIT 3 ALLOW FILTERING fell through the same way, and its after-the-fact transposition then emitted LIMIT 3 LIMIT 500 ALLOW FILTERING.

Changes Made

src/lib/sql/grammar.ts - the dialect grammar declares the clauses, so no branch on a type id exists anywhere in src/lib/db:

  • New fact trailingLimitClauses, one entry per clause, each a pattern matching the clause at the very END of a statement.
  • postgres row: AS OF [AT LEAST] <t> (Materialize's route through this type-id, measured in the issue's E2E pass). Real PostgreSQL and RisingWave share the row and have no such clause: a statement of theirs ending in those words is refused bound or unbound, so the reading costs neither.
  • cassandra row: ALLOW FILTERING, BYPASS CACHE, USING TIMEOUT <duration>. The last two are ScyllaDB's, not Apache Cassandra 5.0's (5.0 answers mismatched input 'USING' expecting EOF), and declaring them on the shared row costs nothing there either.
  • Every other row stays at the default ([]), the spelling this file uses for a fact it has not established.

src/lib/sql/grammar.ts, pattern guards - the clause patterns accept word-runs, and where a clause carries an argument it is a number, a single-quoted literal or one bracket-free token, never a free expression, so a closing quote or a ) after the argument always ends the match. That keeps three shapes from reading as the statement's clause, each pinned by a test:

  • a clause-shaped run inside a literal: … WHERE note = 'BYPASS CACHE' keeps its bound appended after the literal;
  • an alias followed by statement text: SELECT a AS of FROM t keeps its FROM;
  • a clause inside a subquery: … (SELECT … AS OF 123) t keeps the bound at the end.

The bracket-free restriction is not decoration: the first draft of the USING TIMEOUT pattern took \S+ for the duration, and the guard test caught it swallowing the closing parenthesis of (SELECT … USING TIMEOUT 5s), which spliced the bound into the middle of the subquery. CQL itself has no subquery, but the pattern is text and text can carry one regardless of what the engine would do with it, so the token is now [^()\s]+.

src/lib/db/utils/query-limiter.ts:

  • stripTrailingClauses cuts the declared run off the end of the statement, one clause per pattern, from the end backward, so two clauses keep the order they were written in (… LIMIT 500 USING TIMEOUT 5s BYPASS CACHE). Each pattern is end-anchored and eats at least one word, so the index strictly decreases and the loop terminates.
  • applyQueryLimit places the bound before the re-attached run, verbatim, writer's spacing included, and the trailing trivia still follows (… BYPASS CACHE; -- note -> … LIMIT 500 BYPASS CACHE; -- note). With no declared clause the emission is byte-identical to today's.
  • The end-anchored probes in analyzeUnderGrammar (LIMIT n, FETCH FIRST, OFFSET n) read the clause-stripped body, which is what makes an existing bound followed by a declared clause recognisable. The whole-body probes (ROWNUM, UNION, subquery) keep the full statement.

src/lib/db/providers/sql/cassandra/index.ts: APPENDED_AFTER_ALLOW_FILTERING and the transposition are gone - the shared declaration produces the same output (… WHERE amount > 5 ALLOW FILTERING -> … LIMIT 500 ALLOW FILTERING, pinned by the existing integration tests, byte-identical) - and with it the path that could emit LIMIT 3 LIMIT 500 ALLOW FILTERING. The OFFSET refusal and the line-comment refusal stay as they are.

Docs: docs/editor/query-optimization.md gains "Clauses that must follow the bound" under "Where the bound is placed" (the placement table, the recognition rule, the guards, the no-clause behaviour); docs/providers/cassandra.md §2 now describes the declaration rather than the transposition; docs/providers/influxdb3.md's grammar table gains the trailingLimitClauses row, which its provider-doc test reads back field by field.

Testing

  • The unit tests were written first: 10 of them fail on main (every placement and recognition case); 4 pass on main deliberately, because they pin the guards that must hold on both sides of the change. Now 253 pass in tests/unit/db/query-limiter.test.ts.
  • tests/integration/db/cassandra-provider.test.ts: the existing ALLOW FILTERING rows pass unchanged (the fold is byte-identical), plus BYPASS CACHE, USING TIMEOUT 5s, and LIMIT 10 BYPASS CACHE recognised and not doubled - 180 pass.
  • Mutations, each applied and reverted locally: dropping the postgres declaration fails 3 tests; dropping the cassandra declarations fails 13; making the probes read the raw statement end again fails 3; moving the re-attached run after the trailing trivia fails the one test that distinguishes them. The defect cannot come back silently.
  • bun run lint (0 errors), bun run typecheck, bun run format: clean.
  • bun run test: 1030/1032 files. The 14 failures are the ones that fail the same way on clean main on this machine (etcd tls-handshake, DigitalOcean seed helper; checked 2026-10-04) and are environmental - CI runs them green on main.
  • Line coverage of the three changed source files, measured over their test files: 100% (no uncovered line in grammar.ts, query-limiter.ts, cassandra/index.ts).

Out of scope, noted for a follow-up

PostgreSQL's and MySQL's FOR UPDATE / LOCK IN SHARE MODE also follow the row bound, so a statement ending in one has the same shape. It is not declared here because it was not measured for this issue, and this repository's grammar rule 2 does not guess facts. If measured the same way, it is one more entry on the same rows.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation update
  • Test addition or update

Checklist

  • My code follows the project's code style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective
  • New and existing unit tests pass locally with my changes
  • The required CI test job passes the 100% line-coverage gate (bun run test:coverage and bun run coverage:check)

…ibredb#1398)

Materialize's AS OF and ScyllaDB's BYPASS CACHE and USING TIMEOUT follow the
row bound, so appending it refused every bounded read; an existing bound
followed by one was read as unbounded and doubled. The dialect grammar now
declares the clauses that must follow LIMIT, and the limiter places the bound
before the declared run and reads a bound in front of one. ALLOW FILTERING
joins the declaration and the Cassandra provider's after-the-fact
transposition is gone, which also stops it emitting LIMIT 3 LIMIT 500.
@codecov

codecov Bot commented Oct 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cevheri

cevheri commented Oct 4, 2026

Copy link
Copy Markdown
Member

Thanks for this PR. We are preparing the 0.18.0 release right now, so we will review it after that release is out. Thanks for your patience.

@cevheri cevheri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. The grammar declaration is the right shape, the Cassandra transposition folds away cleanly, and the new tests fail on main and pass here.

One thing has to change before I can merge it:

  1. A line comment in front of the clause swallows the bound. stripTrailingClauses starts each match at \s+, so it also takes the newline that closes a -- or // comment, and the bound is then written inside the comment. SELECT * FROM big -- note\nALLOW FILTERING becomes SELECT * FROM big -- note LIMIT 500\nALLOW FILTERING with wasLimited: true, so the engine runs it unbounded while the badge says capped. On main the Cassandra provider still bounded that statement, so this is a regression there. The same gap makes a commented-out bound count as real: -- LIMIT 5\nALLOW FILTERING is read as already limited to 5. Please cut the stripped body at its code end with readStatementEnd, the way applyQueryLimit already treats trailing trivia, so the comment sits between the bound and the clause and the probes only read code.
    Done when: -- note\nALLOW FILTERING, // note\nBYPASS CACHE and -- note\nAS OF AT LEAST 0 each emit a real bound outside the comment, -- LIMIT 5\nALLOW FILTERING is limited to 500, and each case has a test in query-limiter.test.ts.

@cevheri cevheri added the bug Something isn't working label Oct 6, 2026
…ut of the bound (libredb#1398)

The clause patterns match from whitespace, which also takes the newline that
closes a line comment, so the body left behind after the clause strip ended
inside one: the bound was written into the comment while wasLimited reported
it placed, and a commented-out bound read as real. The stripped body is now
cut at its code end, so the comment sits between the bound and the clause.
@KodYazicam

Copy link
Copy Markdown
Contributor Author

Fixed in 509bd4a. stripTrailingClauses now cuts the stripped body at its code end with readStatementEnd, so a comment the writer put between the code and the clause is re-attached between the bound and the clause, its own closing newline kept: SELECT * FROM big -- note\nALLOW FILTERING emits SELECT * FROM big LIMIT 500 -- note\nALLOW FILTERING. The probes read the code half, so -- LIMIT 5\nALLOW FILTERING is no longer a statement limited to 5 - it is bounded to the row limit with the commented-out bound kept as a comment. All four asked cases plus // note\nBYPASS CACHE and a real bound before the comment are pinned in query-limiter.test.ts (they fail on the previous commit); a block comment between code and clause, forceLimit through a comment, and the existing ALLOW FILTERING spacing pins were checked too. 600 tests across the limiter, grammar, Cassandra integration and the influxdb3 doc pin pass; lint, typecheck and format clean.

@cevheri

cevheri commented Oct 6, 2026

Copy link
Copy Markdown
Member

thanks Batuhan, merging shortly

how was your contributing on libredb-studio
also which database are using on libredb-studio on local or cloud etc..

@cevheri
cevheri merged commit 7654cd9 into libredb:main Oct 6, 2026
23 checks passed
@KodYazicam

KodYazicam commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor Author

thanks Batuhan, merging shortly

how was your contributing on libredb-studio also which database are using on libredb-studio on local or cloud etc..

I've been following the project for a few weeks and started contributing about a week ago. Contributing has been a pleasure: the issues come with measured repros and exact acceptance criteria, so it's clear what "done" means before starting. I use it locally with MySQL. 🙂

@cevheri

cevheri commented Oct 6, 2026

Copy link
Copy Markdown
Member

thanks Batuhan, merging shortly
how was your contributing on libredb-studio also which database are using on libredb-studio on local or cloud etc..

I have been following the project for about three weeks and use it locally with MySQL. Happy to be contributing. 🙂

mysql database agent just works with plan mode, agent-atuo mode is not working, because I did not implement mysql readonly query yet. maybe you can implement mysql readonly query implementation. for example postgres, sqlite work with agent auto and plan mode

@KodYazicam

Copy link
Copy Markdown
Contributor Author

Sure, happy to take that on. I'll follow how postgres and sqlite do it and bring MySQL up with them - shout if you'd rather have an issue filed for it first.

@KodYazicam

Copy link
Copy Markdown
Contributor Author

And the same would go for MariaDB if you want it covered too - happy to take that as well.

@cevheri

cevheri commented Oct 6, 2026

Copy link
Copy Markdown
Member

thanks Batuhan

@KodYazicam

Copy link
Copy Markdown
Contributor Author

You're welcome! Ping me on the issue whenever you file it, so I see it right away.

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Query limiter: the row limit is appended after a trailing clause, so Materialize AS OF and ScyllaDB BYPASS CACHE / USING TIMEOUT queries always fail

2 participants