Repository navigation
fix(limiter): place the bound before the dialect's trailing clauses (#1398) - #1501
Conversation
…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 Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
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
left a comment
There was a problem hiding this comment.
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:
- A line comment in front of the clause swallows the bound.
stripTrailingClausesstarts 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 FILTERINGbecomesSELECT * FROM big -- note LIMIT 500\nALLOW FILTERINGwithwasLimited: 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 FILTERINGis read as already limited to 5. Please cut the stripped body at its code end withreadStatementEnd, the wayapplyQueryLimitalready 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 CACHEand-- note\nAS OF AT LEAST 0each emit a real bound outside the comment,-- LIMIT 5\nALLOW FILTERINGis limited to 500, and each case has a test inquery-limiter.test.ts.
…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.
|
Fixed in 509bd4a. |
|
thanks Batuhan, merging shortly how was your contributing on libredb-studio |
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. 🙂 |
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 |
|
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. |
|
And the same would go for MariaDB if you want it covered too - happy to take that as well. |
|
thanks Batuhan |
|
You're welcome! Ping me on the issue whenever you file it, so I see it right away. |
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.
Expected end of statement, found LIMIT, ScyllaDBline 1:38 : Syntax errorforBYPASS CACHEandline 1:42forUSING TIMEOUT 5s.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 FILTERINGfell through the same way, and its after-the-fact transposition then emittedLIMIT 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 insrc/lib/db:trailingLimitClauses, one entry per clause, each a pattern matching the clause at the very END of a statement.postgresrow: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.cassandrarow:ALLOW FILTERING,BYPASS CACHE,USING TIMEOUT <duration>. The last two are ScyllaDB's, not Apache Cassandra 5.0's (5.0 answersmismatched input 'USING' expecting EOF), and declaring them on the shared row costs nothing there either.[]), 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:… WHERE note = 'BYPASS CACHE'keeps its bound appended after the literal;SELECT a AS of FROM tkeeps itsFROM;… (SELECT … AS OF 123) tkeeps the bound at the end.The bracket-free restriction is not decoration: the first draft of the
USING TIMEOUTpattern 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:stripTrailingClausescuts 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.applyQueryLimitplaces 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.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_FILTERINGand 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 emitLIMIT 3 LIMIT 500 ALLOW FILTERING. TheOFFSETrefusal and the line-comment refusal stay as they are.Docs:
docs/editor/query-optimization.mdgains "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 thetrailingLimitClausesrow, which its provider-doc test reads back field by field.Testing
main(every placement and recognition case); 4 pass onmaindeliberately, because they pin the guards that must hold on both sides of the change. Now 253 pass intests/unit/db/query-limiter.test.ts.tests/integration/db/cassandra-provider.test.ts: the existingALLOW FILTERINGrows pass unchanged (the fold is byte-identical), plusBYPASS CACHE,USING TIMEOUT 5s, andLIMIT 10 BYPASS CACHErecognised and not doubled - 180 pass.postgresdeclaration fails 3 tests; dropping thecassandradeclarations 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 cleanmainon this machine (etcd tls-handshake, DigitalOcean seed helper; checked 2026-10-04) and are environmental - CI runs them green onmain.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 MODEalso 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
Checklist
bun run test:coverageandbun run coverage:check)