Fix should allow column list before ENGINE in CREATE MATERIALIZED VIEW#270
Conversation
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 GitHub
Codex 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".
65dcafc to
89db3b8
Compare
89db3b8 to
4167164
Compare
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 GitHub
Codex 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".
… 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
4167164 to
61f8edc
Compare
Problem
Two issues with
CREATE MATERIALIZED VIEWparsing:1. Column list before ENGINE fails (new syntax support)
ClickHouse
SHOW CREATE TABLEfor refreshable materialized views withENGINE(e.g. Memory) outputs the column list between REFRESH and ENGINE:Before:
line 2:0 unexpected token: "(", expected TO or ENGINEAfter: 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
DependsOnslice.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
Changes
parser/parser_view.go(as column list before ENGINE; addconsumeToken()for comma in DEPENDS ONparser/ast.goTableSchema *TableSchemaClausetoCreateMaterializedViewparser/format.goTableSchemabeforeENGINEparser/walk.goTableSchemain WalkTests
Added 2 test fixtures:
create_materialized_view_rmv_engine_with_columns.sql— REFRESH + column list + ENGINE + SETTINGS + DEFINER + COMMENT + subquerycreate_materialized_view_rmv_depends_on_multi.sql— REFRESH + multi-table DEPENDS ON + ENGINEValidated 21 MV syntax variants covering all combinations. All existing tests pass.