feat(editor): apply the row cap as a LIMIT on executed queries#1804
feat(editor): apply the row cap as a LIMIT on executed queries#1804datlechin wants to merge 2 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 708e9488cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard !inString, !inBlockComment, !inDollarQuote, parenDepth == 0, lastCodeEnd > 0 else { return nil } | ||
| return lastCodeEnd |
There was a problem hiding this comment.
Insert Cassandra LIMIT before ALLOW FILTERING
For Cassandra connections, which are registered with .limit auto-limiting, this always appends the cap at the physical end of the statement. A valid CQL query such as SELECT * FROM users WHERE email = 'x' ALLOW FILTERING is rewritten to ... ALLOW FILTERING LIMIT 501, but ALLOW FILTERING is the trailing clause and LIMIT must come before it, so row-capped executions of these common Cassandra selects now fail instead of falling back to the post-fetch cap.
Useful? React with 👍 / 👎.
| if !inString, ch == dash, i + 1 < length, buffer.character(at: i + 1) == dash { | ||
| inLineComment = true | ||
| i += 2 | ||
| continue | ||
| } |
There was a problem hiding this comment.
Keep injected LIMIT out of MySQL hash comments
In MySQL/MariaDB, # starts a line comment, but the scanner only recognizes -- and /* ... */. With a query like SELECT * FROM huge_table # inspect rows, the injector appends LIMIT 501 after the hash comment, so the server executes the original uncapped SELECT and the MySQL driver still has to stream/drain the full result set, defeating the new server-side row cap for this valid MySQL comment form.
Useful? React with 👍 / 👎.
| : SQLParameterExtractor.convertToNativeStyle(sql: stmtSQL, parameters: parameters, style: style) | ||
| let statementSQL = conversion?.sql ?? stmtSQL | ||
|
|
||
| let plan = resolveExecutionPlan(sql: statementSQL, tabType: tabType) |
There was a problem hiding this comment.
Honor without-limit for parameterized batches
When Execute Query Without Limit is used on a selection containing multiple parameterized statements, bypassRowLimit is passed into executeParameterizedAfterSafeMode but this batch path drops it and calls resolveExecutionPlan with the default bypassLimit: false. As a result any SELECT in that parameterized batch still gets LIMIT cap+1 injected, so the new command does not actually bypass the cap for this scenario.
Useful? React with 👍 / 👎.
… the query row cap (#1794)
|
Review pass on this branch surfaced 8 confirmed findings. All are fixed in 123e6fe, plus the lower-priority items the review noted. Correctness fixes
Also picked up
Docs and CHANGELOG updated to match the corrected semantics. ABI check against main reports only the three additive symbols ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 123e6fe47d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let baseQuery = hasBoundParameters | ||
| ? tab.content.query |
There was a problem hiding this comment.
Sort the executed parameterized statement, not the editor
When a parameterized result was produced from a selected statement or the statement at the cursor in an editor that contains other SQL, this branch discards the stored executed query and builds the sort override from tab.content.query (the entire editor). Because runQuery() consumes sortExecutionOverride directly instead of re-selecting the cursor statement, clicking a column header can rerun unrelated statements or append ORDER BY to the wrong SQL rather than sorting the result set the user is viewing.
Useful? React with 👍 / 👎.
Closes #1794.
Problem
The row cap trimmed results only after the full result set crossed the wire. PostgreSQL materializes the whole result in libpq before the app sees a row, and MySQL streams but drains everything up to the 5,000,000-row emergency ceiling. A forgotten LIMIT still pulled millions of rows. On top of that, Execute All Statements bypassed the cap entirely, and a query starting with a comment slipped past both the cap and Safe Mode classification.
What changed
LIMIT (cap+1)appended to the SQL sent to the server. The editor text never changes. Thecap+1over-fetch keeps the existing post-fetch cap, the "truncated / Fetch All" status bar link, and Fetch All (which stores the pre-injection SQL) working unchanged.SQLLimitInjectorscans with paren depth, string, comment, and dollar-quote awareness. A user-written top-levelLIMIT,OFFSET, orFETCHalways wins. An inner LIMIT in a CTE or subquery does not suppress injection. Insertion lands before trailing comments and semicolons. Statements ending inFOR UPDATE,INTO,LOCK, ClickHouseFORMAT/SETTINGSbail out to the post-fetch cap.AutoLimitStylemetadata: MySQL, PostgreSQL, SQLite, ClickHouse, DuckDB, Cassandra, Cloudflare D1, and libSQL get injection; Redis, MongoDB, etcd resolve.none; SQL Server and Oracle keep post-fetch capping only. A new additive PluginKit hookinjectRowLimit(_:limit:)(default nil) lets those plugins ownTOP/FETCH FIRSTinsertion later without an ABI change.MenuwithprimaryAction) with Execute Without Limit; the same command is in the Query menu with Cmd+Option+Return (customizable). Settings footer copy and docs updated.MainContentCoordinator+MultiStatement.swiftproxy (zero callers) and deduplicated the two multi-statement loops into shared helpers.Design note
This reverses the v0.37.0 removal of SQL rewriting (#956). That removal fixed a naive detector that clobbered user-written LIMITs. The new detector only ever appends and never touches a statement that constrains itself; the
ExecuteUserQueryTestsboundary contract (SQL at the driver boundary runs verbatim) still holds because injection happens before that boundary.ABI
Plugins/TableProPluginKit/PluginDatabaseDriver.swiftgains one protocol requirement with a default implementation.scripts/check-pluginkit-abi.sh mainreports exactly the two addedinjectRowLimitlines, nothing removed or changed, so no version bump. Labelabi-additiveapplied.Tests
SQLLimitInjectorTests(21 cases): existing-LIMIT detection, CTE and subquery inner LIMITs, trailing comments, UNION forms, dollar quotes, escaped quotes, identifier false positives, unsupported trailing clauses, malformed input.QueryExecutorTests: row-cap qualification including comment-prefixed SELECTs;QueryClassifierTests: comment-prefixed write/destructive classification.ExecuteUserQueryTests: new case documenting the byte-for-byte boundary contract for post-injection SQL; existing cases unchanged.DriverPluginMetadataTests: registry defaults declare the expected auto-limit style per dialect.swiftlint --strictclean on changed app files.No UI automation added: the without-limit flow needs a live database connection, which TableProUITests cannot drive deterministically.
Known flake (pre-existing, not touched by this change):
QueryExecutorTests/parseSchemaMetadataExtractsEnumValuesfails in parallel batch runs and passes alone.