Skip to content

fix(export): follow the source engine's dialect when exporting query results as SQL - #2632

Merged
datlechin merged 1 commit into
mainfrom
fix/sql-export-dialect
Sep 3, 2026
Merged

fix(export): follow the source engine's dialect when exporting query results as SQL#2632
datlechin merged 1 commit into
mainfrom
fix/sql-export-dialect

Conversation

@datlechin

Copy link
Copy Markdown
Member

Fixes #2630.

What was wrong

Three defects, all one theme: a dialect-dependent decision taken without the dialect that was already in hand. SQLExportPlugin derives SqlDialect.from(databaseTypeId:) in six places and writes the correct -- Database Type: MySQL header from the same field, which is why the header was right while the body was wrong.

1. The query-results export had no driver. ExportDialog.swift:1103 was the only one of three export modes that built its ExportService without one; its initializer was even commented "no driver needed". QueryResultExportDataSource then fell back to SQLEscaping for both quoteIdentifier and escapeStringLiteral.

2. DROP ... CASCADE was emitted unconditionally, for every engine and both export paths, and for views, types and sequences as well as tables.

3. Both query paths asked for structure and drop they cannot deliver. ExportService.swift:252/:320 built the export table with [structure, drop, data] all true while fetchTableDDL returns "", so the dump carried DROP TABLE <the source table> with no CREATE TABLE after it. The issue's own pasted output shows exactly that.

Measured, not assumed

Against a live MariaDB 12.3 and sqlite3:

statement MariaDB 12.3 SQLite
DROP TABLE IF EXISTS "fields" CASCADE; rejected, ERROR 1064 ... near '"fields" CASCADE' rejected, near "CASCADE": syntax error
DROP TABLE IF EXISTS `fields` CASCADE; accepted rejected
DROP TABLE IF EXISTS "fields"; rejected accepted
INSERT INTO "fields" ("id") VALUES (1); rejected n/a

The report blamed two things and one is innocent. MySQL's DROP TABLE grammar does list [RESTRICT | CASCADE], and the manual says outright: "The RESTRICT and CASCADE keywords do nothing. They are permitted to make porting easier." The double quotes are the whole syntax error on MySQL. CASCADE is still a real bug elsewhere: SQLite rejects it, and Drop is on by default, so the ordinary File > Export SQL dump to SQLite has been un-importable too.

A second defect on the same line that nobody reported, and it is worse. The same nil driver dropped to ANSI string escaping. C:\temp\next (hex 433a5c74656d705c6e657874, 12 bytes) exported today re-imports into MariaDB as 433A09656D700A657874, 10 bytes: \t became a tab and \n a newline. Silent corruption, discovered only when someone restores.

Before and after, same server

BEFORE  ERROR 1064 ... near '"fields" CASCADE'          the dump does not import
AFTER   imports clean; HEX(path) = 433A5C74656D705C6E657874 (12 bytes)
        byte-identical to the original C:\temp\next

The fix

Patch for 1 and 2, refactor for 3.

  • The driver is passed at the one call site: DatabaseManager.driver(for:), the installed handle, no lease and no liveness requirement (CLAUDE.md: the driver "is never nil'd"). The common path is now the engine's own exact functions.
  • The fallback stops meaning PostgreSQL. It resolves through the existing resolveSQLDialect + quoteIdentifierFromDialect, which already handles SQL Server's [/]. escapeStringLiteralFromDialect was extracted from SQLRowToStatementConverter's private copy rather than written a second time, and it escapes the same nine characters the MySQL driver does, so a dump taken with a driver and one taken without are the same file.
  • CASCADE follows DriverPlugin.supportsCascadeDrop, the capability that already drives the sidebar's Cascade checkbox. PostgreSQL, Redshift, CockroachDB, PGlite, Snowflake and Dameng keep the clause; every other engine loses one that was either a no-op or a syntax error. The default is false, the answer that is never a syntax error.
  • Query exports are data-only, cleared by column id. The positions differ per format: SQL declares [structure, drop, data], MQL declares [drop, indexes, data], so clearing index 0 and 1 would have turned off MQL's indexes and left its drop on.

One collateral fix rides along because the fix depends on it: Snowflake declared requiresBackslashEscapingInLiterals on its driver and nothing on either descriptor, so the two paths escaped the same value differently. Dameng cannot be fixed the same way, because it detects its escaping at connect time; that is documented in the helper rather than papered over.

Review

Codex is out of credits until Sep 7 (You've hit your usage limit), so the second-model review was Skill(code-review), not Codex. It found 15 issues; 11 are fixed here.

The most valuable one caught a fix of mine that did not work. I had declared Snowflake's backslash escaping on the curated snapshot, but buildMetadataSnapshot takes editor.sqlDialect from driverType.sqlDialect and replaces the curated one outright once the plugin loads. Snowflake is registry-only, so the plugin is always installed for real use: the fix was inert, and my test passed only because plugins never load under XCTest. It is now declared on SnowflakePlugin.swift too, and the test says which one it covers.

Also fixed from the review: the two query data sources carried a dead supportsCascadeDrop that was the only reason their inits needed @MainActor; a dead stored driver; a doc comment orphaned onto the wrong function; a tautological test that asserted the value it had just passed in; a mismatched-length defaults array read positionally; and a settings race where my copied export harness and the existing one both did capture-reset-restore over the same UserDefaults key while Swift Testing runs suites in parallel. Both suites now share one serialized harness.

Verification

  • Build PASS. swiftlint --strict over TablePro, Plugins, TableProTests: 0 violations.
  • 15 new tests; 60 pass across the export and dialect suites, 35 across the MCP export suites.
  • ABI gate: additive. One requirement added, carrying a default; no removals; PluginExportDataSource has no conformer in any plugin bundle. No version bump, no plugin re-release.
  • docs: house style and source claims agree.
  • SQLExport and MQLExport both build. AllPlugins fails locally on the known oracle-nio @TaskLocal macro issue (both errors from that package); CI covers it.

No UI automation: nothing in this change is reachable from a deterministic UI flow, since the defect is in the text of a written file rather than in a control.

Behaviour change worth knowing

A query-results SQL export is now INSERT-only. That is correct, and the option tree is hidden in that mode so nobody chose otherwise, but it is visible.

For an ad-hoc query the INSERT names the export's file name, which for a query tab defaults to query_results and so names no real table. That was equally true before, alongside a DROP of the same name, so this is strictly an improvement rather than a regression, but it is not yet right; renaming the file renames the table. Reported rather than fixed, because a result set genuinely has no table behind it and picking one is a product decision.

https://claude.ai/code/session_011EqgjCjCAU6tiiVmnMpF86

@mintlify

mintlify Bot commented Sep 3, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
TablePro 🟢 Ready View Preview Sep 3, 2026, 5:43 PM

💡 Tip: Enable Automations to automatically generate PRs for you.

@datlechin
datlechin merged commit cd1fc5d into main Sep 3, 2026
9 checks passed
@datlechin
datlechin deleted the fix/sql-export-dialect branch September 3, 2026 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Export results syntax is wrong for MySQL databases

1 participant