Fix should allow column list before ENGINE in CREATE MATERIALIZED VIEW - #270
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65dcafc43c
ℹ️ About Codex in GitHubCodex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41671648a4
ℹ️ About Codex in GitHubCodex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Sorry, something went wrong.
… in CREATE MATERIALIZED VIEW
1. Column list before ENGINE (new):
ClickHouse SHOW CREATE TABLE for refreshable materialized views with
ENGINE (e.g. Memory) outputs the column list between REFRESH and ENGINE:
CREATE MATERIALIZED VIEW db1.mv_name
REFRESH EVERY 1 SECOND
(col1 String, col2 Int8)
ENGINE = Memory
AS SELECT ...
The parser previously expected TO or ENGINE immediately after REFRESH
clauses. Added support for (columns) before ENGINE by parsing a
TableSchemaClause when a left paren is encountered.
2. DEPENDS ON multi-table (bug fix):
The comma in 'DEPENDS ON db1.mv_a, db1.mv_b' was not consumed before
parsing the next table identifier. Added missing consumeToken() call
in the comma loop, matching the pattern used in parser_query.go.
Changes:
- parser/parser_view.go: handle (columns) before ENGINE; consume comma in DEPENDS ON loop
- parser/ast.go: add TableSchema field to CreateMaterializedView
- parser/format.go: emit TableSchema before ENGINE in FormatSQL
- parser/walk.go: traverse TableSchema in Walk
Tests:
- create_materialized_view_rmv_engine_with_columns.sql (column list + ENGINE)
- create_materialized_view_rmv_depends_on_multi.sql (multi-table DEPENDS ON)
- All 21 MV syntax variants tested, all existing tests pass
Problem
Two issues with CREATE MATERIALIZED VIEW parsing:
1. Column list before ENGINE fails (new syntax support)
ClickHouse SHOW CREATE TABLE for refreshable materialized views with ENGINE (e.g. Memory) outputs the column list between REFRESH and ENGINE:
Before: line 2:0 unexpected token: "(", expected TO or ENGINE
After: Parses correctly. Column list stored in CreateMaterializedView.TableSchema.
2. DEPENDS ON with multiple tables fails (bug fix)
Before: expected <ident> or <string>, but got ","
After: Parses correctly. Both tables captured in DependsOn slice.
Root cause: The comma token was not consumed before parsing the next table identifier in the DEPENDS ON loop. The query parser (parser_query.go) has the correct pattern (consumeToken() after matching comma), but the view parser was missing it.
ClickHouse syntax reference
CREATE MATERIALIZED VIEW [IF NOT EXISTS] [db.]name [ON CLUSTER cluster] [REFRESH EVERY|AFTER interval [OFFSET interval]] [RANDOMIZE FOR interval] [DEPENDS ON [db.]name [, [db.]name [, ...]]] [SETTINGS name = value [, ...]] [APPEND] [TO [db.]name] [(columns)] [ENGINE = engine] [EMPTY] [DEFINER = { user | CURRENT_USER }] [SQL SECURITY { DEFINER | NONE }] AS SELECT ... [COMMENT 'comment']Changes
Tests
Added 2 test fixtures:
Validated 21 MV syntax variants covering all combinations. All existing tests pass.