refactor(storage): make remaining sqlstore SQL dialect-portable - #1007
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors pkg/storage/internal/sqlstore to remove remaining MySQL-specific SQL constructs by routing them through the Dialect interface or rewriting them using more portable SQL patterns, with the intent of enabling additional storage backends without rewriting query call sites.
Changes:
- Added new
Dialectseams forInsertIfAbsent,JSONBooleanIsTrue, andIndexHint, and wired call sites to use them. - Replaced MySQL-only SQL in query text (e.g.,
TIMESTAMPDIFF,SUBSTRING_INDEX,FORCE INDEX, JSON boolean comparisons) with portable SQL and/or Go-side computation. - Updated store wiring so
applyCommentStoreuses the injected dialect rather than a hard-coded MySQL dialect.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/storage/internal/sqlstore/webhook_events.go | Replaces TIMESTAMPDIFF-based age computation with portable selection + Go duration calculation. |
| pkg/storage/internal/sqlstore/storage.go | Wires applyCommentStore to use deps.Dialect (dialect seam) instead of MySQLDialect{}. |
| pkg/storage/internal/sqlstore/dialect.go | Extends Dialect and implements MySQL versions of InsertIfAbsent, JSONBooleanIsTrue, and IndexHint. |
| pkg/storage/internal/sqlstore/dialect_test.go | Adds unit tests for the new MySQL dialect methods. |
| pkg/storage/internal/sqlstore/apply_operations.go | Replaces SUBSTRING_INDEX and moves JSON boolean predicate behind Dialect.JSONBooleanIsTrue. |
| pkg/storage/internal/sqlstore/apply_comments.go | Replaces INSERT IGNORE with Dialect.InsertIfAbsent(...) fragments. |
| pkg/storage/internal/sqlstore/applies.go | Replaces FORCE INDEX with Dialect.IndexHint(...) and threads dialect through target-lock checks/claim paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
morgo
left a comment
There was a problem hiding this comment.
🤖 Approving on Morgan's behalf (automated review, escalation rules apply).
Checked each rewrite for MySQL equivalence:
SUBSTRING_INDEX(k,'/',1)→CASE WHEN POSITION... THEN k ELSE SUBSTRING(...): equivalent including the no-slash (whole key) and leading-slash (empty prefix) edges; the new slash-less finalizer integration test covers the branch that matters.defer_cutovergate:JSONBooleanIsTruerenders the same null-safe<=> CAST('true' AS JSON)predicate modulo the quoted path ($."defer_cutover"), which MySQL treats identically; unit test pins the exact SQL.InsertIfAbsent: renders the sameINSERT IGNORE, and the affected-rows contract (1 on insert, 0 on conflict) matches how ClaimSummaryComment decides the winner.IndexHint: same FORCE INDEX, now backtick-escaped.InboxStats: age subtraction moves into Go with the same NULL/zero handling — metrics-only path.
Non-blocking note: #1006 edits the adjacent storage.go constructor lines (planComments/settings vs. applyComments here), so whichever merges second will likely need a trivial rebase.
Move INSERT IGNORE, JSON boolean predicates, and the index hint behind the dialect; rewrite TIMESTAMPDIFF and SUBSTRING_INDEX portably. No behavior change for MySQL.
JSONBooleanIsTrue previously mixed Go and SQL escaping, which would mis-resolve non-identifier keys once a second caller appeared. The contract now requires plain identifier keys and panics otherwise. Also refreshes dialect-seam comments that described MySQL-only rendering, and pins the slash-less operation-key branch of the group finalizer gate with an integration test.
54f3ad2 to
8570883
Compare
…k' into kiran01bm/pg-backend-nucleus * origin/kiran01bm/sqlstore-joined-dml-13k: refactor(storage): reject bind placeholders in JoinedUpdate join conditions refactor(storage): tighten the JoinedUpdate dialect contract refactor(storage): render joined UPDATEs through the dialect feat(postgres): implement declarative planning via pg-sprite diffplan (#1008) refactor(storage): make remaining sqlstore SQL dialect-portable (#1007) test(e2e): deflake multi-table stop/start resume and MySQL cold starts (#1005) ci: verify golangci config against a vendored schema (#997) feat(api): fail-closed verdict gating for postgres plans (#1004) feat(tern): route postgres targets to the postgres engine (#1003) feat(storage): stamp remaining sqlstore timestamps explicitly (#1006) # Conflicts: # pkg/storage/internal/sqlstore/apply_comments.go # pkg/storage/internal/sqlstore/dialect.go # pkg/storage/internal/sqlstore/dialect_test.go # pkg/storage/internal/sqlstore/settings.go # pkg/storage/internal/sqlstore/storage.go # pkg/storage/internal/sqlstore/updated_at_lint_test.go
…-joined-dml-13l * origin/main: refactor(storage): render joined UPDATEs through the dialect (#1009) fix(planetscale): hold the cutover when the operator defers it (#978) fix(observability): make telemetry resource schema-tolerant (#1014) feat(postgres): implement declarative planning via pg-sprite diffplan (#1008) refactor(storage): make remaining sqlstore SQL dialect-portable (#1007) test(e2e): deflake multi-table stop/start resume and MySQL cold starts (#1005) ci: verify golangci config against a vendored schema (#997) feat(api): fail-closed verdict gating for postgres plans (#1004) feat(tern): route postgres targets to the postgres engine (#1003) feat(storage): stamp remaining sqlstore timestamps explicitly (#1006) # Conflicts: # pkg/storage/internal/sqlstore/apply_comments.go # pkg/storage/internal/sqlstore/dialect.go # pkg/storage/internal/sqlstore/dialect_test.go # pkg/storage/internal/sqlstore/settings.go # pkg/storage/internal/sqlstore/storage.go # pkg/storage/internal/sqlstore/updated_at_lint_test.go
…re-public-13n * origin/main: refactor(storage): portable lease-guarded joined DML for the operation store (#1011) fix(github): align lint warnings formatting with issues and fold long lists (#959) fix(github): lead with the database's operators on command-rejection comments (#960) docs: regenerate stale tables of contents (#968) fix(engine): heartbeat the row a local drive actually owns (#915) fix(github): scope auto-plan to the schema a pull request proposes (#1016) fix(vitess): dispatch task-less VSchema-only work operations over gRPC (#961) feat(storage): add PostgreSQL dialect nucleus to the shared store core (#1010) refactor(storage): render joined UPDATEs through the dialect (#1009) fix(planetscale): hold the cutover when the operator defers it (#978) fix(observability): make telemetry resource schema-tolerant (#1014) feat(postgres): implement declarative planning via pg-sprite diffplan (#1008) refactor(storage): make remaining sqlstore SQL dialect-portable (#1007) test(e2e): deflake multi-table stop/start resume and MySQL cold starts (#1005) ci: verify golangci config against a vendored schema (#997) feat(api): fail-closed verdict gating for postgres plans (#1004) feat(tern): route postgres targets to the postgres engine (#1003) feat(storage): stamp remaining sqlstore timestamps explicitly (#1006)
…lect-factory-14b * origin/main: feat(github): flag destructive changes to tables another open PR owns (#1017) feat(storage): add public postgresstore constructor (#1012) refactor(storage): portable lease-guarded joined DML for the operation store (#1011) fix(github): align lint warnings formatting with issues and fold long lists (#959) fix(github): lead with the database's operators on command-rejection comments (#960) docs: regenerate stale tables of contents (#968) fix(engine): heartbeat the row a local drive actually owns (#915) fix(github): scope auto-plan to the schema a pull request proposes (#1016) fix(vitess): dispatch task-less VSchema-only work operations over gRPC (#961) feat(storage): add PostgreSQL dialect nucleus to the shared store core (#1010) refactor(storage): render joined UPDATEs through the dialect (#1009) fix(planetscale): hold the cutover when the operator defers it (#978) fix(observability): make telemetry resource schema-tolerant (#1014) feat(postgres): implement declarative planning via pg-sprite diffplan (#1008) refactor(storage): make remaining sqlstore SQL dialect-portable (#1007) test(e2e): deflake multi-table stop/start resume and MySQL cold starts (#1005) ci: verify golangci config against a vendored schema (#997) feat(api): fail-closed verdict gating for postgres plans (#1004) feat(tern): route postgres targets to the postgres engine (#1003) feat(storage): stamp remaining sqlstore timestamps explicitly (#1006)
Summary
Removes the last MySQL-only SQL constructs from
pkg/storage/internal/sqlstoreby moving them behind the storageDialectseam or rewriting them portably. No behavior change for MySQL.Why
The sqlstore is being decoupled from MySQL so a second storage dialect can implement the same interface without rewriting query call sites. These were the remaining hard spots where MySQL syntax leaked into shared query text; each is now either dialect-rendered or genuinely portable SQL.
What
INSERT IGNORE→Dialect.InsertIfAbsent(conflictColumns), which returns the modifier/suffix fragments around a portableINSERT INTO ... VALUES. The contract requires 1 affected row on insert and 0 on conflict, so callers keep their existing affected-rows checks.TIMESTAMPDIFF(MICROSECOND, ...)in webhook inbox stats → selectMIN(received_at)and the database's current timestamp, compute the age in Go.SUBSTRING_INDEX→ portablePOSITION/SUBSTRING/CASEexpression.JSON_EXTRACT ... <=> CAST('true' AS JSON)) →Dialect.JSONBooleanIsTrue(expr, path), defined to yield false for missing paths, JSON null, SQL NULL, and non-boolean values. Path keys must be plain identifiers; implementations panic otherwise.FORCE INDEXin the active-apply target check →Dialect.IndexHint(index); dialects without hint syntax return an empty string.Before / after