Skip to content

make D1 splitter.ts use sqlite's parser logic - #15712

Merged
alsuren merged 3 commits into
mainfrom
dlaban/15228-make-splitter-use-sqlite-parser-logic
Sep 23, 2026
Merged

alsuren merged 3 commits into
mainfrom
dlaban/15228-make-splitter-use-sqlite-parser-logic

Conversation

@alsuren

@alsuren alsuren commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #15228 and a bunch of others

There are a bunch of MRs (#15234 #15226 and #15163) that are trying to fix minor bugs in our sqlite parser in splitter.ts.

The fundamental problem with that parser is that it's derived from a generic sql parser that is primarily a PostgreSQL parser rather than a sqlite parser.

I wrote on one of them what the solution is: #15163 (comment)

If we are in the business of writing sql parsers, I think we would be better off faithfully porting the logic from https://github.com/sqlite/sqlite/blob/master/src/complete.c rather than organically trying to recreate it one edge case at a time (maybe translate the state machine to use strings rather than integers for the states?).

That way, each time we get a bug report about our parser, we can point an LLM at the upstream sqlite source code and ask "is this a faithful translation from c to typescript?".

(an LLM based review pointed out that we don't handle EXPLAIN correctly and we don't handle [..] style quotes. I understand that neither of these are very common, and neither are regressions introduced in your MR)

The other advantage would be that workerd already carries a patch to make complete.c expose a sqlite_complete_length() function, which we use to split statements (for use with wrangler d1 execute --remote --file f.sql). If the implementations of these statement splitters are are as similar as possible then we will have fewer discrepancies there too.

The existing code also some funkiness around removing comments, but I know that it is due to this bug: #7739 . We don't need to remove all comments to work around that workerd bug: we can just remove fragments that only contain whitespace and comments.

I asked my agent to include the regression tests from #15234 #15226 and #15163 in our test suite, to make sure their edge-cases are covered.


  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: trivial bugfixes

A picture of a cute animal (not mandatory, but encouraged)

@changeset-bot

changeset-bot Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ffbcdb8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
wrangler Patch
@cloudflare/vite-plugin Patch
@cloudflare/vitest-plugin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pkg-pr-new

pkg-pr-new Bot commented Sep 18, 2026 •

Copy link
Copy Markdown
@cloudflare/autoconfig

npm i https://pkg.pr.new/@cloudflare/autoconfig@15712

@cloudflare/build-output-utils

npm i https://pkg.pr.new/@cloudflare/build-output-utils@15712

@cloudflare/codemods

npm i https://pkg.pr.new/@cloudflare/codemods@15712

@cloudflare/config

npm i https://pkg.pr.new/@cloudflare/config@15712

@cloudflare/containers-shared

npm i https://pkg.pr.new/@cloudflare/containers-shared@15712

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@15712

@cloudflare/deploy-helpers

npm i https://pkg.pr.new/@cloudflare/deploy-helpers@15712

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@15712

miniflare

npm i https://pkg.pr.new/miniflare@15712

@cloudflare/pages-functions

npm i https://pkg.pr.new/@cloudflare/pages-functions@15712

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@15712

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@15712

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@15712

@cloudflare/vitest-plugin

npm i https://pkg.pr.new/@cloudflare/vitest-plugin@15712

@cloudflare/workers-auth

npm i https://pkg.pr.new/@cloudflare/workers-auth@15712

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@15712

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@15712

wrangler

npm i https://pkg.pr.new/wrangler@15712

commit: ffbcdb8

@ask-bonk

ask-bonk Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor
  1. P1: Unterminated quoted SQL fragments are silently discarded.

hasUsefulToken remains false when scanning fails at an opening [ or quote. Therefore SELECT 1; [unterminated and SELECT 1; 'unterminated execute only SELECT 1 locally and report success. Preserve those fragments so SQLite reports the invalid statement.

Bracketed identifier:

				return { end: undefined, hasUsefulToken: true };

Quoted value or identifier:

				return { end: undefined, hasUsefulToken: true };

@ask-bonk

ask-bonk Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

I'm Bonk, and I've done a quick review of your PR.

PR #15712 replaces D1 SQL splitting with SQLite's statement-completion state machine.

  1. P1: Unterminated quoted SQL fragments can be silently discarded after a valid statement, allowing partial local execution. Suggested fixes posted: make D1 splitter.ts use sqlite's parser logic #15712 (comment)

github run

@alsuren
alsuren force-pushed the dlaban/15228-make-splitter-use-sqlite-parser-logic branch from fe3fc29 to ffbcdb8 Compare September 18, 2026 17:10
@alsuren
alsuren marked this pull request as ready for review September 20, 2026 16:35
@workers-devprod
workers-devprod requested review from a team and cjol and removed request for a team September 20, 2026 16:35
@workers-devprod

workers-devprod commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

Codeowners approval required for this PR:

  • ✅ @cloudflare/d1
  • ✅ @cloudflare/wrangler
Show detailed file reviewers

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

Devin Review

Comment thread packages/wrangler/src/d1/splitter.ts

@cjol cjol 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.

You’re right, this seems a much better fix than the piecemeal PRs we’ve received. I’ll hold off on the others in favour of this?

@workers-devprod workers-devprod left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Codeowners reviews satisfied

brandhaug added a commit to brandhaug/b2b-saas-starter that referenced this pull request Sep 27, 2026
## pnpm-workspace.yaml (default)

## Dependency Updates

| Package | From | To | Type |
| --- | --- | --- | --- |
| `wrangler` | 4.136.2 | 4.138.0 | minor |

## Release Notes

<details>
<summary><b>wrangler</b> (4.136.2 → 4.138.0) — 3 releases</summary>

<details>
<summary><b>4.138.0</b></summary>

### Minor Changes

- [#15776](cloudflare/workers-sdk#15776)
[`b03f960`](cloudflare/workers-sdk@b03f960)
Thanks [@<!---->edevil](https://github.com/edevil)! - Add event-code
support to temporary Worker deployments

Use `wrangler deploy --temporary --event-code <code>` to provision an
account for an event. Wrangler requires explicit server acknowledgement
before caching the account and keeps the event code out of its cache and
telemetry.

- [#15817](cloudflare/workers-sdk#15817)
[`6e77c53`](cloudflare/workers-sdk@6e77c53)
Thanks [@<!---->jamesopstad](https://github.com/jamesopstad)! - Allow
framework commands to produce Preview Build Output with the experimental
config

When `cf previews deploy` invokes a framework build command, Preview
intent is now preserved. Function-based `cloudflare.config.ts` files
receive `isPreview: true`, and generated Build Output is marked as a
Preview build.

### Patch Changes

- [#15806](cloudflare/workers-sdk#15806)
[`8fade73`](cloudflare/workers-sdk@8fade73)
Thanks [@<!---->NuroDev](https://github.com/NuroDev)! - Standardize Zod
validation error output

Format validation errors with Zod's built-in `prettifyError()` helper so
Miniflare, Wrangler, the Vite plugin, and the Vitest plugin show
consistent messages and property paths.

- Updated dependencies
[[`a71237a`](cloudflare/workers-sdk@a71237a),
[`8fade73`](cloudflare/workers-sdk@8fade73)]:
  - miniflare@5.20260921.1-alpha

</details>

<details>
<summary><b>4.137.0</b></summary>

### Minor Changes

- [#15778](cloudflare/workers-sdk#15778)
[`cd7508c`](cloudflare/workers-sdk@cd7508c)
Thanks [@<!---->jamesopstad](https://github.com/jamesopstad)! - Generate
types during development and supported builds with Vite's
`experimental.newConfig` option or Wrangler's
`--experimental-new-config` flag (and `--experimental-cf-build-output`
for builds)

When Wrangler's `--experimental-new-config` flag or Vite's
`experimental.newConfig` option is enabled, inferred configuration and
runtime declarations are now kept in `.cloudflare/types/index.d.ts`.
Vite refreshes them during development and production builds. Wrangler
refreshes them during development and when building with both
`--experimental-new-config` and `--experimental-cf-build-output`. In the
experimental `wrangler.config.ts` format, the `types` option is now
top-level because it applies to both commands.

### Patch Changes

- [#15765](cloudflare/workers-sdk#15765)
[`1bdb96d`](cloudflare/workers-sdk@1bdb96d)
Thanks [@<!---->th0m](https://github.com/th0m)! - Prepare the required
egress sidecar for local Containers without configured images

Wrangler dev and Vite dev/preview now pull the required sidecar for
Durable Object-managed Containers that select their application image at
start time. Previously, these Containers failed to start unless the
sidecar image was already cached in Docker.

- [#15712](cloudflare/workers-sdk#15712)
[`f5605f5`](cloudflare/workers-sdk@f5605f5)
Thanks [@<!---->alsuren](https://github.com/alsuren)! - Match D1 SQL
statement splitting to the local SQLite runtime

Wrangler now uses SQLite's statement-completion state machine when
splitting D1 SQL files. This keeps trigger, quoted identifier, comment,
and keyword handling consistent with local exe

…[full
notes](https://github.com/cloudflare/workers-sdk/releases/tag/wrangler%404.137.0)

</details>

<p><i>…and 1 more release(s) not shown</i></p>

</details>

---
*This PR was auto-generated by
[catalog-update-action](https://github.com/brandhaug/catalog-update-action).*

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[wrangler] D1 SQL splitter does not treat bracket-quoted identifiers as quoted

4 participants