Mz deploy fixes#37142
Draft
sjwiesman wants to merge 6 commits into
Draft
Conversation
…ansaction
`apply tables` failed when applying a newly-created `CREATE TABLE ... FROM
SOURCE` that carried a companion `GRANT` (or `COMMENT`):
transactions which modify objects are restricted to just modifying objects
(AdapterError::DDLOnlyTransaction)
`CreateTableFromSource` statements are grouped by source into a single
BEGIN/COMMIT so that multiple tables from one source are created together. The
Created path drained the CREATE, its indexes, and the reconciled grants/comments
into one `ObjectResult.statements`, so the grouped transaction wrapped the GRANT
alongside the DDL. Materialize forbids mixing a GRANT/COMMENT with object
creation in one transaction, so the apply aborted.
This was latent because the existing grant test only grants on a pre-existing
table, where the grant runs via the UpToDate path (standalone, no transaction).
Add an `ObjectResult.post_statements` bucket that `ApplyPlan::execute` runs after
the transaction commits, outside any BEGIN/COMMIT. `apply tables` now keeps only
the bare CREATE in the grouped `statements` and defers indexes, grants, and
comments to `post_statements`. The table is still reported as a single `created`
result, so per-phase action counts are unchanged. The other apply commands set
the new field empty and are unaffected.
Tests: the `basic/v2` incremental fixture's `products` table now carries a
companion grant, so the `apply-incremental` mzcompose case exercises
create-plus-grant in one apply and asserts the grant landed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…work
Objects whose name is a SQL reserved word (`table`, `order`, `select`, …)
were unusable end to end:
- A file `table.sql` containing `CREATE TABLE "table"` failed validation
with `ObjectNameMismatch`: `validate_ident` compared the statement name
rendered through `Ident`'s `AstDisplay` (which re-quotes the keyword to
`"table"`) against the unquoted file stem `table`.
- A declared external dependency `db.schema."table"` never resolved:
`ObjectId::from_str` split on `.` and kept the literal quotes, so the
declared form matched neither the typecheck lookup nor `lock`'s catalog
query (the catalog stores the unquoted `table`).
The root cause was that an object's identity was stored as a `String`
produced inconsistently — quoted (`Ident::to_string()`) when derived from
SQL, unquoted when derived from file stems and the catalog.
Store the identity as `Ident` instead, throughout `ObjectId` and
`DatabaseIdent`. `Ident`'s `Eq`/`Hash`/`Ord` compare the raw identifier
value, so the quoted/unquoted mismatch is impossible by construction;
quoting now happens only at the edges (Display, serialization, error
messages). `ObjectId::new` keeps its `String` signature (wrapping inputs
with `Ident::new_unchecked`) and the accessors keep returning `&str`, so the
~140 construction and read sites are unchanged. `from_str` now parses with
the SQL parser so quoted/reserved-word components are handled correctly, and
`from_item_name` stores the parsed `Ident` directly. The change is a no-op
for every non-keyword identifier (an `Ident` only renders differently from
its raw value when it needs quoting), so the blast radius is reserved-word
names only.
Tests: unit tests in `object_id` cover quoted parsing, quoted-Display
round-tripping, and equality across the construction paths; a new
`reserved-words` mzcompose workflow applies a `CREATE TABLE "table"` project
(path matching) and locks a reserved-word external dependency (catalog
resolution).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ship the reserved-words test project's types.lock alongside the other test projects, and refresh the system-deps lock to the current catalog rendering (the single-byte `"char"` type is reported quoted). Both workflows delete and regenerate their lock before asserting, so these checked-in copies are snapshots rather than load-bearing inputs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The error hints suggested a `--name` flag that no longer exists. `stage` takes `--deploy-id` and `abort` takes a positional deploy id, so e.g. a missing deployment now hints `mz-deploy stage . --deploy-id <id>` and the conflict rebase hint uses `mz-deploy abort <staging-env>` (positional). Fixes the DeploymentNotFound, DeploymentConflict, and GitShaFailed hints. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Re-running `mz-deploy init` in a directory that is already a committed git
repo (or already an mz-deploy project) failed with "git commit failed": the
scaffold ran `git commit` unconditionally, which errors when there is nothing
to commit. It also overwrote existing files (e.g. a user's project.toml) and
relied on the ambient git identity for the committer.
Make scaffolding converge instead of fail:
- `add_file` skips files that already exist, so re-runs only create what's
missing and never clobber user content.
- the commit runs only when `git diff --cached --quiet` reports staged
changes, so an already-committed tree is a no-op rather than an error.
- the commit sets GIT_COMMITTER_NAME/EMAIL explicitly (author was already
hardcoded), so it doesn't depend on the user's git config being set.
Tests: scaffold the same directory twice (git path included) and confirm both
runs succeed, and that an existing project.toml is preserved.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `init` and `new` help pages listed `.agents/skills/mz-deploy/SKILL.md` and a `.claude/skills/mz-deploy/` symlink among the scaffolded files, but the scaffold creates neither. Remove the inaccurate bullets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove these sections if your commit already has a good description!
Motivation
Why does this change exist? Link to a GitHub issue, design doc, Slack
thread, or explain the problem in a sentence or two. A reviewer who has
no context should understand why after reading this section.
If this implements or addresses an existing issue, it's enough to link to that:
Closes
Fixes
etc.
Description
What does this PR actually do? Focus on the approach and any non-obvious
decisions. The diff shows the code --- use this space to explain what the
diff can't tell a reviewer.
Verification
How do you know this change is correct? Describe new or existing automated
tests, or manual steps you took.