Fix parsing failure when having multiple ttl expressions #234
Conversation
Pull Request Test Coverage Report for Build 21353959011Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
Updates ALTER TABLE parsing to support multiple TTL expressions in MODIFY TTL clauses.
Changes:
- Parse
ALTER TABLE ... MODIFY TTLusing aTTLClause(list ofTTLExpr) rather than a singleTTLExpr. - Update AST shape/printing for
AlterTableModifyTTLto reflect the clause-based TTL representation. - Add test coverage (SQL + golden JSON + format fixture) for multiple TTL expressions in
MODIFY TTL.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| parser/parser_alter.go | Switch MODIFY TTL parsing to build a TTLClause with multiple TTL items. |
| parser/ast.go | Change AlterTableModifyTTL.TTL to *TTLClause and adjust String() output accordingly. |
| parser/testdata/dml/output/alter_table_with_modify_ttl.sql.golden.json | Update expected AST JSON to match the new TTLClause structure for a single TTL item. |
| parser/testdata/dml/output/alter_table_modify_ttl_multiple.sql.golden.json | Add golden AST JSON for a multi-item MODIFY TTL statement. |
| parser/testdata/dml/alter_table_modify_ttl_multiple.sql | Add new SQL test input for multiple TTL expressions in MODIFY TTL. |
| parser/testdata/dml/format/alter_table_modify_ttl_multiple.sql | Add format/roundtrip fixture for the new multi-TTL SQL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| MODIFY TTL | ||
| toDateTime(timestamp / 1000000000) + INTERVAL 30 DAY TO DISK 'gcs', | ||
| toDateTime(timestamp / 1000000000) + INTERVAL 60 DAY; |
There was a problem hiding this comment.
This new test covers multiple TTL expressions, but it doesn’t cover the case where the last TTL expression has a TTL policy (e.g., ... + INTERVAL 60 DAY TO DISK 'gcs'). Adding such a case would help ensure the multi-item parsing/AST boundaries behave correctly for the common TO DISK/TO VOLUME variants.
| ModifyPos Pos | ||
| StatementEnd Pos | ||
| TTL *TTLExpr | ||
| TTL *TTLClause |
There was a problem hiding this comment.
AlterTableModifyTTL.TTL changed from *TTLExpr to *TTLClause, which is a breaking change for any external consumers of this package’s AST. If this repo aims to keep API stability, consider preserving the old field (e.g., add a new TTLClause field while keeping TTLExpr for the single-item case), or provide a helper accessor/migration path and document the breaking change (potentially requiring a major version bump).
| TTL *TTLClause | |
| TTL *TTLClause | |
| // TTLExpr is kept for backwards compatibility with older versions where | |
| // TTL was of type *TTLExpr. New code should use TTL (*TTLClause). | |
| TTLExpr *TTLExpr |
| listEnd := ttlPos | ||
| if len(items) > 0 { | ||
| listEnd = items[len(items)-1].End() | ||
| } |
There was a problem hiding this comment.
parseTTLClause always appends the first parsed TTL expression before returning, so len(items) can’t be 0 on the success path. The fallback listEnd := ttlPos and the if len(items) > 0 branch are therefore redundant and can be simplified.
| listEnd := ttlPos | |
| if len(items) > 0 { | |
| listEnd = items[len(items)-1].End() | |
| } | |
| listEnd := items[len(items)-1].End() |
| listEnd := ttlPos | ||
| if len(items) > 0 { | ||
| listEnd = items[len(items)-1].End() | ||
| } | ||
| ttlClause := &TTLClause{ | ||
| TTLPos: ttlPos, | ||
| ListEnd: listEnd, | ||
| Items: items, | ||
| } | ||
| return &AlterTableModifyTTL{ | ||
| ModifyPos: pos, | ||
| StatementEnd: ttlExpr.End(), | ||
| TTL: ttlExpr, | ||
| StatementEnd: ttlClause.End(), | ||
| TTL: ttlClause, |
There was a problem hiding this comment.
listEnd is derived from items[len(items)-1].End(), but TTLExpr.End() currently ignores any attached TTL policy (e.g., ... TO DISK 'x', ... DELETE, etc.). If the last TTL item has a policy, AlterTableModifyTTL.StatementEnd/TTLClause.ListEnd will point before the policy tokens, making node ranges inconsistent. Consider updating TTLExpr.End() (in parser/ast.go) to return Policy.End() when Policy != nil, and then use that for ListEnd here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09c27f44d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case p.matchKeyword(KeywordTtl): | ||
| ttlPos := p.Pos() | ||
| _ = p.lexer.consumeToken() | ||
| ttlExpr, err := p.parseTTLExpr(p.Pos()) | ||
| items, err := p.parseTTLClause(ttlPos, true) | ||
| if err != nil { |
There was a problem hiding this comment.
With allowMultiValues set to true, parseTTLClause greedily consumes any comma as another TTL expression separator. In ALTER TABLE, commas also separate alter clauses, so a statement like ALTER TABLE t MODIFY TTL x, ADD COLUMN y Int now fails because parseTTLClause will attempt to parse ADD as part of a TTL expression and error. This makes valid multi-clause ALTER statements containing MODIFY TTL unparsable; consider stopping TTL parsing when the next token starts another alter clause (ADD/DROP/...) or only enabling multi-values when MODIFY TTL is the final clause.
Useful? React with 👍 / 👎.
3 participants
No description provided.