Skip to content

feat(explain): save and diff query plans - #2380

Open
sophiathedev wants to merge 3 commits into
TableProApp:mainfrom
sophiathedev:feat/explain-plan-history
Open

feat(explain): save and diff query plans#2380
sophiathedev wants to merge 3 commits into
TableProApp:mainfrom
sophiathedev:feat/explain-plan-history

Conversation

@sophiathedev

Copy link
Copy Markdown
Contributor

Summary

  • Save successful EXPLAIN output with query history and expose compatible earlier runs from the plan viewer.
  • Compare baseline/current summary metrics and report deterministic added, removed, and modified plan nodes.
  • Fall back to bounded side-by-side raw output when either plan cannot be parsed.

Matching and storage

  • Baselines require the exact subject SQL plus connection, database, schema, database type, EXPLAIN variant, and format.
  • Plan payloads live in a child table; the baseline list reads metadata only and loads raw text lazily by UUID.
  • Storage is capped at 2 MB per plan, 100 MB total raw payload, and 1,000 snapshots. Old payloads are pruned without deleting parent history rows.
  • Pause, retention, and deletion follow existing query-history behavior. Snapshot failures degrade to parent-history-only writes.
  • Parameterized EXPLAIN output is not persisted because a database may echo bind values in its raw plan.
  • No PluginKit or public ABI changes.

Flow

flowchart LR
  E[Successful EXPLAIN] --> H[Query history row]
  H -->|FK + cascade| S[Plan snapshot]
  S -->|metadata only| L[Baseline list]
  L -->|selected UUID| R[Lazy raw load]
  R --> P[Parse + bounded diff]
  P -->|parse unavailable| F[Raw side-by-side fallback]
Loading

Evidence

  • Focused macOS unit suites: 230 passed, 0 failed, 0 skipped.
  • Strict SwiftLint on changed app sources: 0 violations.
  • macOS and iOS localization catalogs: verified.
  • git diff --check: passed.
  • Added an end-to-end macOS UI test covering explicit EXPLAIN, a changed query plan, immediate history reload, node-change accessibility, and typed EXPLAIN persistence. CI result pending.

Test plan

  • Run the same EXPLAIN before and after adding an index; open History and compare metrics/node changes.
  • Verify explicit and typed EXPLAIN paths both create compatible baselines.
  • Verify parameterized plans keep the history row but do not store raw plan output.
  • Verify migration, pruning, cascade deletion, pause, retention, exact-scope matching, parser fallback, bounded comparison work, and localization coverage.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

@datlechin

Copy link
Copy Markdown
Member

Rebuilt the feature on top of this branch (d5de0733a), after a multi-lane investigation of the design. The idea is right and the storage skeleton was sound; four things were structurally wrong and are now fixed.

What changed and why

1. The comparison is a mode, not a modal sheet.

A macOS sheet is always modal and dims its parent, and the whole point of comparing a plan is to change the query or add an index and run it again. The HIG routes exactly this away from modality ("For complex or prolonged user flows, consider alternatives to sheets"), and this repo already states the rule in CompareSyncWindowController.swift:6-19, which quotes "Avoid creating custom window UI" and "Avoid putting critical information or actions in a bottom bar". The sheet had both.

Compare is now a fourth mode beside Diagram / Tree / Raw, with the baseline chooser in the pane's own bar. That is the shape of Xcode's own comparison editor: a mode, a revision picker, and the diff in place.

2. Baselines match on the statement's fingerprint, not its exact text.

p.subject_query = ? meant reformatting or re-indenting a query silently started a new history, which defeats the headline use case. SQLQueryFingerprint already normalizes a statement the way pg_stat_statements does, and every history row already stores and indexes the same hash. Baselines now key on it, so reformatting and changing a literal both keep the chain. Switching EXPLAIN to EXPLAIN ANALYZE still starts a separate one, because they report different things.

3. A saved plan is not a child of a history row.

query_plan_snapshots had ON DELETE CASCADE to history(id), so ordinary history retention deleted plans, including one the user deliberately kept. The new plan_snapshots table owns its own lifetime: history_id is nullable provenance with ON DELETE SET NULL, plans survive clearing and pruning history, and a plan can be pinned to exempt it from cleanup entirely.

Pruning also moved out of the insert. Sharing one transaction with the history row meant a failure on the large write took the small one down with it: SQLite auto-rolls back on SQLITE_FULL, the explicit ROLLBACK then fails with "cannot rollback - no transaction is active", and the history entry was lost along with the plan. Plan writes are now a separate statement, and pruning runs on query history's own cleanup cadence.

4. The diff uses the standard library, and reports a verdict.

The 300-line hand-rolled LCS with a bit-packed skip table is replaced by CollectionDifference (current.difference(from: baseline)), which is the same algorithm with a proven implementation and no 1,000,000-cell cap. The comparison is a pure QueryPlanDiff.compare(baseline:current:) rather than work inside an init, so it is testable and runs off the main actor without a view attached.

It now leads with what happened, before the metric table: "3.4x slower than the baseline", "The plan shape changed", "No measurable change". A timing difference under 15% counts as noise, so two runs of an unchanged plan no longer read as a regression.

Defects fixed along the way

  • queryHistoryDidUpdate carries UUID? where nil documented-means "every connection" (QuickSwitcherCatalogStore.swift:59-61). The branch sent nil on an ordinary record whenever a plan was attached, so every EXPLAIN invalidated the quick-switcher catalog for every connection and refreshed every open history panel. Restored to the connection id.
  • Node metric values reached the UI through String(describing:), so a cost rendered as 52000000.0 right under a summary that spelled the same number 52,000,000. Values stay numeric until they are drawn, and format through FormatStyle.
  • QueryPlanLabels.visibleProperties drops any value spelled 0 or false. Reusing it for the diff reported Rows Removed by Filter falling from 1000 to 0, the exact improvement the reader came for, as the property being removed.
  • The plan-history context was built independently in both EXPLAIN paths, from different sources for the database name. One shared builder now serves both, so a typed EXPLAIN and the Explain action land in one chain.
  • The 150 ms Task.sleep before every baseline selection is gone; the reload is scoped to the connection and debounced the way the history drawer and insights tab already are.
  • The fabricated __typed_explain__:<sha256> variant id is replaced by a readable normalized preamble, so a stored key is debuggable and showable.
  • A parameterized query still stores no plan, but Compare now says so instead of showing an empty list.
  • Added/removed/changed carry a glyph as well as a tint and honour accessibilityDifferentiateWithoutColor, matching StructureDefinitionDiffView.
  • The raw fallback goes through the repo's own DiffComputer, so an unparseable plan is shown as a real line diff rather than two blobs to align by eye.

Collateral, outside this feature

  • .gitignore:186 had an unanchored Localization/, which also matched TableProTests/Localization/. The string-catalog guard suite existed on disk and was invisible to git and CI. Anchored to /Localization/, so it ships and runs.
  • That guard's printf parser did not understand Foundation's %#@name@ plural substitutions, so it under-counted a plural source by one argument and reported five correct translations as defects. Fixed, with cases covering it.
  • With those five gone, one real defect remained: the Vietnamese Charting the first %1$@ of %2$@ loaded rows passed %1$d where the source passes %@. Fixed.

Verification

  • Build: PASS
  • 276 unit cases across the plan, storage, classifier, history, diff and catalog suites: 276 passed, 0 failed
  • swiftlint --strict on every changed Swift file: 0 violations
  • verify.sh docs: PASS
  • The UI test is rewritten for the mode, and asserts that a typed EXPLAIN and the Explain action build one history rather than two. It has not been run locally; CI result pending.
  • Localization catalog: 20 new keys with all five languages, 10 orphans removed, and three keys that shipped Korean-only completed.

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.

2 participants