---
phase: 45-alter-comment-get-ddl
plan: 01
type: execute
wave: 1
depends_on: []
files_modified:
  - src/parse.rs
  - src/ddl/alter.rs
  - src/lib.rs
  - test/sql/phase45_alter_comment.test
  - test/sql/TEST_LIST
autonomous: true
requirements: [ALT-01, ALT-02]

must_haves:
  truths:
    - "ALTER SEMANTIC VIEW v SET COMMENT = 'text' changes the view-level comment"
    - "ALTER SEMANTIC VIEW v UNSET COMMENT removes the view-level comment"
    - "ALTER SEMANTIC VIEW IF EXISTS nonexistent SET COMMENT = 'x' is a silent no-op"
    - "ALTER SEMANTIC VIEW IF EXISTS nonexistent UNSET COMMENT is a silent no-op"
    - "Changed/removed comments are visible in subsequent SHOW SEMANTIC VIEWS and DESCRIBE SEMANTIC VIEW"
  artifacts:
    - path: "src/parse.rs"
      provides: "Generalized Alter/AlterIfExists DdlKind variants with sub-operation dispatch for RENAME TO, SET COMMENT, UNSET COMMENT"
      contains: "DdlKind::Alter"
    - path: "src/ddl/alter.rs"
      provides: "AlterSetCommentVTab and AlterUnsetCommentVTab with catalog read-modify-write + persistence"
      contains: "AlterSetCommentVTab"
    - path: "src/lib.rs"
      provides: "Registration of 4 new table functions for SET/UNSET COMMENT (with and without IF EXISTS)"
      contains: "alter_semantic_view_set_comment"
    - path: "test/sql/phase45_alter_comment.test"
      provides: "sqllogictest integration tests for ALTER SET/UNSET COMMENT"
    - path: "test/sql/TEST_LIST"
      provides: "Updated test list including phase45_alter_comment.test"
  key_links:
    - from: "src/parse.rs"
      to: "src/ddl/alter.rs"
      via: "DDL rewrite to table function call"
      pattern: "alter_semantic_view_set_comment"
    - from: "src/ddl/alter.rs"
      to: "src/catalog.rs"
      via: "catalog_upsert for in-memory update"
      pattern: "catalog_upsert"
    - from: "src/lib.rs"
      to: "src/ddl/alter.rs"
      via: "register_table_function_with_extra_info"
      pattern: "AlterSetCommentVTab"
---

<objective>
Implement ALTER SEMANTIC VIEW SET/UNSET COMMENT DDL commands that modify the view-level comment after creation.

Purpose: Users need to add, change, or remove descriptive comments on semantic views without recreating them. This mirrors Snowflake's ALTER SEMANTIC VIEW SET/UNSET COMMENT syntax.
Output: Parser detection + validation for SET COMMENT and UNSET COMMENT operations, table function implementations for catalog mutation, persistence, and integration tests.
</objective>

<execution_context>
@$HOME/.claude/get-shit-done/workflows/execute-plan.md
@$HOME/.claude/get-shit-done/templates/summary.md
</execution_context>

<context>
@.planning/PROJECT.md
@.planning/ROADMAP.md
@.planning/STATE.md
@.planning/phases/45-alter-comment-get-ddl/45-RESEARCH.md

@src/parse.rs
@src/ddl/alter.rs
@src/ddl/persist.rs
@src/catalog.rs
@src/model.rs
@src/lib.rs
@test/sql/phase34_1_alter_rename.test

<interfaces>
<!-- Key types and contracts the executor needs. Extracted from codebase. -->

From src/parse.rs:
```rust
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum DdlKind {
    Create, CreateOrReplace, CreateIfNotExists,
    Drop, DropIfExists,
    Describe, Show, ShowTerse, ShowColumns,
    AlterRename, AlterRenameIfExists,  // RENAME THESE to Alter/AlterIfExists
    ShowDimensions, ShowMetrics, ShowFacts,
}

fn function_name(kind: DdlKind) -> &'static str { ... }
fn detect_ddl_prefix(trimmed: &str) -> Option<(DdlKind, usize)> { ... }
fn rewrite_ddl(query: &str) -> Result<String, String> { ... }
pub fn validate_and_rewrite(query: &str) -> Result<Option<String>, ParseError> { ... }
```

From src/ddl/alter.rs:
```rust
pub struct AlterRenameState {
    pub catalog: CatalogState,
    pub persist_conn: Option<ffi::duckdb_connection>,
    pub if_exists: bool,
}
pub struct AlterRenameVTab; // impl VTab
```

From src/catalog.rs:
```rust
pub type CatalogState = Arc<RwLock<HashMap<String, String>>>;
pub fn catalog_upsert(state: &CatalogState, name: &str, json: &str) -> Result<(), Box<dyn std::error::Error>>;
```

From src/model.rs:
```rust
pub struct SemanticViewDefinition {
    pub comment: Option<String>,  // view-level comment (Phase 43)
    // ... other fields
}
```
</interfaces>
</context>

<tasks>

<task type="auto" tdd="true">
  <name>Task 1: Generalize ALTER DdlKind and add SET/UNSET COMMENT parser support</name>
  <files>src/parse.rs</files>
  <read_first>
    - src/parse.rs (current DdlKind enum, detect_ddl_prefix, function_name, rewrite_ddl, validate_and_rewrite -- understand all ALTER handling)
  </read_first>
  <behavior>
    - Test: detect_ddl_kind("ALTER SEMANTIC VIEW v SET COMMENT = 'test'") returns Some(DdlKind::Alter)
    - Test: detect_ddl_kind("ALTER SEMANTIC VIEW IF EXISTS v SET COMMENT = 'test'") returns Some(DdlKind::AlterIfExists)
    - Test: detect_ddl_kind("ALTER SEMANTIC VIEW v UNSET COMMENT") returns Some(DdlKind::Alter)
    - Test: detect_ddl_kind("ALTER SEMANTIC VIEW IF EXISTS v UNSET COMMENT") returns Some(DdlKind::AlterIfExists)
    - Test: detect_ddl_kind("ALTER SEMANTIC VIEW v RENAME TO w") returns Some(DdlKind::Alter) (backwards compat -- same variant, different sub-operation)
    - Test: validate_and_rewrite("ALTER SEMANTIC VIEW v SET COMMENT = 'hello'") returns Ok(Some("SELECT * FROM alter_semantic_view_set_comment('v', 'hello')"))
    - Test: validate_and_rewrite("ALTER SEMANTIC VIEW v UNSET COMMENT") returns Ok(Some("SELECT * FROM alter_semantic_view_unset_comment('v')"))
    - Test: validate_and_rewrite("ALTER SEMANTIC VIEW IF EXISTS v SET COMMENT = 'hello'") returns Ok(Some("SELECT * FROM alter_semantic_view_set_comment_if_exists('v', 'hello')"))
    - Test: validate_and_rewrite("ALTER SEMANTIC VIEW IF EXISTS v UNSET COMMENT") returns Ok(Some("SELECT * FROM alter_semantic_view_unset_comment_if_exists('v')"))
    - Test: validate_and_rewrite("ALTER SEMANTIC VIEW v RENAME TO w") still returns Ok(Some("SELECT * FROM alter_semantic_view_rename('v', 'w')")) (unchanged behavior)
    - Test: validate_and_rewrite("ALTER SEMANTIC VIEW v TRUNCATE") returns Err with message about supported operations
    - Test: validate_and_rewrite("ALTER SEMANTIC VIEW v SET COMMENT = 'it''s a test'") correctly handles escaped quotes, rewrites to set_comment('v', 'it''s a test')
    - Test: validate_and_rewrite("ALTER SEMANTIC VIEW v") returns Err about missing operation
  </behavior>
  <action>
    Refactor the ALTER DdlKind variants and extend parser for SET COMMENT and UNSET COMMENT.

    **Step 1: Rename DdlKind variants** (per research recommendation):
    - Rename `DdlKind::AlterRename` to `DdlKind::Alter`
    - Rename `DdlKind::AlterRenameIfExists` to `DdlKind::AlterIfExists`
    - Update all match arms across parse.rs referencing these variants (detect_ddl_prefix, function_name, rewrite_ddl, validate_and_rewrite, extract_ddl_name)
    - Note: function_name() cannot simply return a single string for Alter anymore since the target function depends on the sub-operation. Instead, the function_name for Alter/AlterIfExists will no longer be used -- the rewrite_ddl match arm for Alter will compute the function name directly.

    **Step 2: Extend validate_and_rewrite for ALTER**:
    In the `DdlKind::Alter | DdlKind::AlterIfExists` match arm of `validate_and_rewrite`:
    - After extracting the view name (everything between the DDL prefix and the next whitespace), examine the rest of the string
    - Convert `rest` to uppercase for keyword matching
    - If rest starts with `RENAME TO`: validate new_name present (existing logic), delegate to rewrite
    - If rest starts with `SET COMMENT`: extract `= '<comment_text>'` using single-quote parsing that handles `''` escaping (reuse the extract_view_comment pattern from line 873), validate format, delegate to rewrite
    - If rest starts with `UNSET COMMENT`: validate nothing else follows (or just trailing whitespace/semicolons), delegate to rewrite
    - Otherwise: return ParseError with message "Unsupported ALTER operation. Supported: RENAME TO, SET COMMENT, UNSET COMMENT."

    **Step 3: Extend rewrite_ddl for ALTER**:
    In the `DdlKind::Alter | DdlKind::AlterIfExists` match arm of `rewrite_ddl`:
    - After extracting view name and rest, detect sub-operation from rest_upper:
    - If `RENAME TO`: existing logic, function name = `alter_semantic_view_rename` / `alter_semantic_view_rename_if_exists`
    - If `SET COMMENT = '...'`: extract the comment string (handle `''` escaping in input, preserve `''` in output for SQL safety), function name = `alter_semantic_view_set_comment` / `alter_semantic_view_set_comment_if_exists`, rewrite to `SELECT * FROM fn_name('view_name', 'comment_text')`
    - If `UNSET COMMENT`: function name = `alter_semantic_view_unset_comment` / `alter_semantic_view_unset_comment_if_exists`, rewrite to `SELECT * FROM fn_name('view_name')`
    - Otherwise: error "Unsupported ALTER operation"

    **Step 4: Update function_name**:
    For `DdlKind::Alter` and `DdlKind::AlterIfExists`, return a generic placeholder (e.g., `"alter_semantic_view"`) since the actual function name is now computed in rewrite_ddl. Or better: remove these from function_name entirely and handle the mapping in the rewrite_ddl match arm. The function_name helper is only called once in rewrite_ddl with `let fn_name = function_name(kind)` at line 530 -- but the ALTER arm never uses fn_name (it computes its own). So just keep Alter/AlterIfExists returning a dummy string that is never used, or restructure to avoid calling function_name for ALTER variants.

    **Step 5: Update extract_ddl_name**:
    Update the match for `AlterRename | AlterRenameIfExists` to `Alter | AlterIfExists`.

    **Comment extraction for SET COMMENT**: The `=` sign is followed by a single-quoted string. Parse the quoted string handling `''` -> `'` escaping (identical to what extract_view_comment does in parse.rs lines 873-910). For the rewrite output, the comment value needs `'` -> `''` re-escaping since it will be embedded in a SQL string literal in the rewrite.

    Write unit tests for all behaviors listed above. Add them in the existing `#[cfg(test)] mod tests` block within parse.rs, near the existing alter_rename tests.
  </action>
  <verify>
    <automated>cargo test -- --test-threads=1 parse 2>&1 | tail -20</automated>
  </verify>
  <acceptance_criteria>
    - src/parse.rs contains `DdlKind::Alter` and `DdlKind::AlterIfExists` (NOT `AlterRename`/`AlterRenameIfExists`)
    - src/parse.rs contains string `alter_semantic_view_set_comment` in rewrite logic
    - src/parse.rs contains string `alter_semantic_view_unset_comment` in rewrite logic
    - src/parse.rs contains string `alter_semantic_view_set_comment_if_exists` in rewrite logic
    - src/parse.rs contains string `alter_semantic_view_unset_comment_if_exists` in rewrite logic
    - src/parse.rs validate_and_rewrite match arm for Alter handles RENAME TO, SET COMMENT, and UNSET COMMENT
    - Error message for unsupported ALTER operations contains "RENAME TO, SET COMMENT, UNSET COMMENT"
    - All existing parse.rs tests pass (including alter_rename tests which now use Alter variant)
    - New tests for SET COMMENT and UNSET COMMENT parse/rewrite pass
    - `cargo test` exits 0
  </acceptance_criteria>
  <done>DdlKind generalized from AlterRename to Alter, parser detects SET COMMENT and UNSET COMMENT sub-operations, rewrite produces correct table function calls, all unit tests pass</done>
</task>

<task type="auto">
  <name>Task 2: AlterComment VTab implementation, registration, and integration tests</name>
  <files>src/ddl/alter.rs, src/lib.rs, test/sql/phase45_alter_comment.test, test/sql/TEST_LIST</files>
  <read_first>
    - src/ddl/alter.rs (AlterRenameVTab pattern to replicate)
    - src/ddl/persist.rs (execute_parameterized function signature)
    - src/catalog.rs (catalog_upsert signature)
    - src/lib.rs (extension init_extension function, AlterRenameState registration block)
    - src/model.rs (SemanticViewDefinition struct, comment field)
    - test/sql/phase34_1_alter_rename.test (test format to follow)
  </read_first>
  <action>
    **Step 1: Add AlterCommentState and AlterSetCommentVTab to src/ddl/alter.rs**:

    Below the existing AlterRenameVTab code, add:

    ```rust
    use crate::catalog::{catalog_rename, catalog_upsert, CatalogState};
    use crate::model::SemanticViewDefinition;
    ```
    (Update the existing import line to add `catalog_upsert` and add the model import.)

    Add a new state struct:
    ```rust
    #[derive(Clone)]
    pub struct AlterCommentState {
        pub catalog: CatalogState,
        pub persist_conn: Option<ffi::duckdb_connection>,
        pub if_exists: bool,
    }
    // SAFETY: same as AlterRenameState
    unsafe impl Send for AlterCommentState {}
    unsafe impl Sync for AlterCommentState {}
    ```

    Add persistence helper:
    ```rust
    fn persist_comment_update(conn: ffi::duckdb_connection, name: &str, json: &str) {
        unsafe {
            let _ = super::persist::execute_parameterized(
                conn,
                "UPDATE semantic_layer._definitions SET definition = $1 WHERE name = $2",
                &[json, name],
            );
        }
    }
    ```

    Add `AlterSetCommentVTab`:
    - Bind data: `name: String, comment: String`
    - In bind():
      1. Declare output columns: `name` (VARCHAR), `status` (VARCHAR)
      2. Read parameters: name (0), comment (1)
      3. Get AlterCommentState from extra_info
      4. Read existing JSON from catalog via `state.catalog.read().unwrap().get(&name).cloned()`
      5. If None and state.if_exists: return bind data with status "no-op"
      6. If None and !state.if_exists: return Err("semantic view 'name' does not exist")
      7. Deserialize JSON to SemanticViewDefinition via `serde_json::from_str`
      8. Set `def.comment = Some(comment)`
      9. Re-serialize to JSON via `serde_json::to_string`
      10. Persist first: call `persist_comment_update(conn, name, new_json)` if persist_conn is Some
      11. Update in-memory catalog: `catalog_upsert(&state.catalog, &name, &new_json)?`
      12. Return bind data with status "comment set"
    - Init and func: identical pattern to AlterRenameVTab (AtomicBool done flag, single-row output)
    - parameters(): One VARCHAR parameter for SET, two VARCHAR for SET_COMMENT (name + comment)
    Wait -- this needs to be two VARCHAR params: name and comment.

    Add `AlterUnsetCommentVTab`:
    - Same as AlterSetCommentVTab but:
      - Only one parameter (name)
      - Sets `def.comment = None` instead of `Some(comment)`
      - Status message: "comment unset"
    - parameters(): One VARCHAR parameter (name only)

    Both VTabs should share the same AlterCommentState type. The difference is one takes 2 params (name + comment) and the other takes 1 param (name).

    Add bind data structs:
    ```rust
    pub struct AlterCommentBindData {
        name: String,
        status: String,
    }
    unsafe impl Send for AlterCommentBindData {}
    unsafe impl Sync for AlterCommentBindData {}
    ```

    Reuse AlterRenameInitData pattern (or create a shared AlterInitData).

    **Step 2: Register in src/lib.rs**:

    In the `extension` module's `init_extension` function, after the existing `alter_semantic_view_rename_if_exists` registration block (around line 446), add 4 new registrations:

    ```rust
    // Register alter_semantic_view_set_comment(name, comment)
    let alter_comment_state = AlterCommentState {
        catalog: catalog_state.clone(),
        persist_conn,
        if_exists: false,
    };
    con.register_table_function_with_extra_info::<AlterSetCommentVTab, _>(
        "alter_semantic_view_set_comment",
        &alter_comment_state,
    )?;

    let alter_comment_if_exists_state = AlterCommentState {
        catalog: catalog_state.clone(),
        persist_conn,
        if_exists: true,
    };
    con.register_table_function_with_extra_info::<AlterSetCommentVTab, _>(
        "alter_semantic_view_set_comment_if_exists",
        &alter_comment_if_exists_state,
    )?;

    // Register alter_semantic_view_unset_comment(name)
    let alter_unset_comment_state = AlterCommentState {
        catalog: catalog_state.clone(),
        persist_conn,
        if_exists: false,
    };
    con.register_table_function_with_extra_info::<AlterUnsetCommentVTab, _>(
        "alter_semantic_view_unset_comment",
        &alter_unset_comment_state,
    )?;

    let alter_unset_comment_if_exists_state = AlterCommentState {
        catalog: catalog_state.clone(),
        persist_conn,
        if_exists: true,
    };
    con.register_table_function_with_extra_info::<AlterUnsetCommentVTab, _>(
        "alter_semantic_view_unset_comment_if_exists",
        &alter_unset_comment_if_exists_state,
    )?;
    ```

    Also add the imports in the `use crate::ddl::alter::` block:
    ```rust
    alter::{AlterCommentState, AlterRenameState, AlterRenameVTab, AlterSetCommentVTab, AlterUnsetCommentVTab},
    ```

    **Step 3: Create sqllogictest at test/sql/phase45_alter_comment.test**:

    Follow the pattern from phase34_1_alter_rename.test. Test cases:

    1. Setup: Create a table and semantic view (with existing COMMENT)
    2. ALTER SEMANTIC VIEW v SET COMMENT = 'new comment' -- verify via SHOW SEMANTIC VIEWS (check comment column) and DESCRIBE SEMANTIC VIEW (check COMMENT property row)
    3. ALTER SEMANTIC VIEW v SET COMMENT = 'updated comment' -- verify overwrites
    4. ALTER SEMANTIC VIEW v UNSET COMMENT -- verify comment is removed (NULL in SHOW, no COMMENT property in DESCRIBE)
    5. ALTER SEMANTIC VIEW v SET COMMENT = 'it''s escaped' -- verify comment with single quotes
    6. ALTER SEMANTIC VIEW IF EXISTS nonexistent SET COMMENT = 'noop' -- verify no error
    7. ALTER SEMANTIC VIEW IF EXISTS nonexistent UNSET COMMENT -- verify no error
    8. ALTER SEMANTIC VIEW nonexistent SET COMMENT = 'fail' -- verify error "does not exist"
    9. ALTER SEMANTIC VIEW nonexistent UNSET COMMENT -- verify error "does not exist"
    10. Verify the view is still queryable after ALTER SET/UNSET COMMENT (query integrity preserved)

    **Step 4: Update test/sql/TEST_LIST**:
    Add `test/sql/phase45_alter_comment.test` as a new line at the end of TEST_LIST.

    **Step 5: Build and run full test suite**:
    Run `just build && just test-sql` to verify sqllogictest passes.
    Run `cargo test` to verify all Rust tests pass.
  </action>
  <verify>
    <automated>just test-all</automated>
  </verify>
  <acceptance_criteria>
    - src/ddl/alter.rs contains `pub struct AlterCommentState`
    - src/ddl/alter.rs contains `pub struct AlterSetCommentVTab`
    - src/ddl/alter.rs contains `pub struct AlterUnsetCommentVTab`
    - src/ddl/alter.rs contains `fn persist_comment_update`
    - src/ddl/alter.rs contains `catalog_upsert` call for in-memory catalog update
    - src/ddl/alter.rs contains `serde_json::from_str` and `serde_json::to_string` for JSON roundtrip
    - src/lib.rs contains `alter_semantic_view_set_comment` registration
    - src/lib.rs contains `alter_semantic_view_unset_comment` registration
    - src/lib.rs contains `alter_semantic_view_set_comment_if_exists` registration
    - src/lib.rs contains `alter_semantic_view_unset_comment_if_exists` registration
    - test/sql/phase45_alter_comment.test exists with at least 8 test cases
    - test/sql/TEST_LIST contains `phase45_alter_comment.test`
    - `just test-all` exits 0
  </acceptance_criteria>
  <done>ALTER SET/UNSET COMMENT fully implemented: parser detects and rewrites, table functions mutate catalog and persistence, integration tests verify SET, UNSET, IF EXISTS, quoting, and error cases, full test suite passes</done>
</task>

</tasks>

<threat_model>
## Trust Boundaries

| Boundary | Description |
|----------|-------------|
| DDL input -> parser | User-provided SQL strings with potential injection |
| Parser -> table function | Rewritten SQL with escaped string values |

## STRIDE Threat Register

| Threat ID | Category | Component | Disposition | Mitigation Plan |
|-----------|----------|-----------|-------------|-----------------|
| T-45-01 | Tampering | ALTER COMMENT string | mitigate | Comment value passes through single-quote escaping in rewrite_ddl (replace ' with '') and is bound as a parameter via table function bind, not string interpolation |
| T-45-02 | Spoofing | ALTER on nonexistent view | mitigate | if_exists flag check in VTab bind(); without IF EXISTS, returns clear error |
| T-45-03 | Denial of Service | Very large comment string | accept | No practical size limit needed -- DuckDB VARCHAR handles large strings; comments are metadata-only |
</threat_model>

<verification>
1. `cargo test` passes (all Rust unit tests including new parse tests)
2. `just build && just test-sql` passes (sqllogictest including phase45_alter_comment.test)
3. `just test-all` passes (full suite)
4. ALTER SET COMMENT visible in SHOW SEMANTIC VIEWS comment column
5. ALTER UNSET COMMENT removes comment (NULL in output)
6. IF EXISTS on nonexistent view is silent no-op
7. Non-IF EXISTS on nonexistent view returns clear error
</verification>

<success_criteria>
- ALT-01: ALTER SEMANTIC VIEW SET COMMENT works end-to-end (parse -> rewrite -> VTab -> persist -> visible in SHOW/DESCRIBE)
- ALT-02: ALTER SEMANTIC VIEW UNSET COMMENT works end-to-end (same flow, sets comment to None)
- IF EXISTS variants are silent no-ops for missing views
- Single-quote escaping handles edge cases correctly
- All existing tests continue to pass (no regressions)
</success_criteria>

<output>
After completion, create `.planning/phases/45-alter-comment-get-ddl/45-01-SUMMARY.md`
</output>
