Skip to content

Fix should allow column list before ENGINE in CREATE MATERIALIZED VIEW#270

Merged
git-hulk merged 1 commit into
AfterShip:masterfrom
sharadgaur:sharad/fix-rmv-column-list-engine
May 15, 2026
Merged

Fix should allow column list before ENGINE in CREATE MATERIALIZED VIEW#270
git-hulk merged 1 commit into
AfterShip:masterfrom
sharadgaur:sharad/fix-rmv-column-list-engine

Conversation

@sharadgaur
Copy link
Copy Markdown
Contributor

@sharadgaur sharadgaur commented May 14, 2026

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:

CREATE MATERIALIZED VIEW db1.config_cache_v0
REFRESH EVERY 1 SECOND
(
    `id` String,
    `name` String,
    `value` Float64
)
ENGINE = Memory
AS SELECT id, name, value FROM db1.config_v0

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)

CREATE MATERIALIZED VIEW db1.mv
REFRESH EVERY 5 MINUTE
DEPENDS ON db1.mv_a, db1.mv_b
ENGINE = Memory
AS SELECT 1

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

File Change
parser/parser_view.go Handle ( as column list before ENGINE; add consumeToken() for comma in DEPENDS ON
parser/ast.go Add TableSchema *TableSchemaClause to CreateMaterializedView
parser/format.go Emit TableSchema before ENGINE
parser/walk.go Traverse TableSchema in Walk

Tests

Added 2 test fixtures:

  • create_materialized_view_rmv_engine_with_columns.sql — REFRESH + column list + ENGINE + SETTINGS + DEFINER + COMMENT + subquery
  • create_materialized_view_rmv_depends_on_multi.sql — REFRESH + multi-table DEPENDS ON + ENGINE

Validated 21 MV syntax variants covering all combinations. All existing tests pass.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread parser/parser_view.go
@sharadgaur sharadgaur force-pushed the sharad/fix-rmv-column-list-engine branch from 65dcafc to 89db3b8 Compare May 14, 2026 19:35
@sharadgaur sharadgaur marked this pull request as draft May 14, 2026 19:43
@sharadgaur sharadgaur force-pushed the sharad/fix-rmv-column-list-engine branch from 89db3b8 to 4167164 Compare May 14, 2026 19:46
@sharadgaur sharadgaur marked this pull request as ready for review May 14, 2026 19:47
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread parser/ast.go
… 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
@sharadgaur sharadgaur force-pushed the sharad/fix-rmv-column-list-engine branch from 4167164 to 61f8edc Compare May 14, 2026 19:50
@git-hulk git-hulk self-requested a review May 15, 2026 02:02
@git-hulk git-hulk changed the title fix: support column list before ENGINE in CREATE MATERIALIZED VIEW Fix should allow column list before ENGINE in CREATE MATERIALIZED VIEW May 15, 2026
@git-hulk git-hulk merged commit 88f462c into AfterShip:master May 15, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants