Fix invalid DDL for multi-column SummingMergeTree - #52
Merged
Conversation
HasSummingMergeTreeEngine with more than one column emitted a comma-separated argument list (SummingMergeTree(`A`, `B`)), which ClickHouse rejects with NUMBER_OF_ARGUMENTS_DOESNT_MATCH — the engine takes a single optional parameter that must be a tuple of columns. Multiple columns are now wrapped in a tuple (SummingMergeTree((`A`, `B`))); single-column and no-column usage are unchanged. The bug slipped through because engine DDL was only covered by string-comparison unit tests (one of which asserted the invalid SQL as correct) and no test executed the generated CREATE TABLE against a real server. Adds engine-argument round-trip integration tests that run EnsureCreated against ClickHouse for SummingMergeTree (single and multiple columns), CollapsingMergeTree, VersionedCollapsingMergeTree, and AggregatingMergeTree, so invalid engine DDL now fails the build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes ClickHouse DDL generation for SummingMergeTree when configured with multiple sum columns by emitting a single tuple parameter (instead of an invalid comma-separated argument list), and adds integration coverage to ensure engine clauses round-trip successfully against a real ClickHouse server.
Changes:
- Update migrations SQL generation to wrap multi-column
SummingMergeTreesum columns in a tuple, while keeping single-column behavior unchanged. - Correct/extend unit tests to assert the tuple form for multi-column
SummingMergeTreeand add a single-column test. - Add new integration tests that execute
EnsureCreatedAsyncfor several MergeTree-family engines to catch invalid engine DDL at runtime. - Document the fix in
CHANGELOG.mdandRELEASENOTES.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.ClickHouse.Tests/MigrationSqlGeneratorTests.cs | Updates the existing unit assertion for multi-column SummingMergeTree and adds a single-column test case. |
| test/EFCore.ClickHouse.Tests/MigrationIntegrationTests.cs | Adds engine-argument round-trip integration tests validating CREATE TABLE succeeds and engine clauses are persisted as expected. |
| src/EFCore.ClickHouse/Migrations/ClickHouseMigrationsSqlGenerator.cs | Fixes SummingMergeTree engine argument emission: tuple-wraps multiple sum columns. |
| RELEASENOTES.md | Notes the SummingMergeTree multi-column DDL bug fix in the unreleased section. |
| CHANGELOG.md | Notes the SummingMergeTree multi-column DDL bug fix in the unreleased section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alex-clickhouse
force-pushed
the
fix/summing-mergetree-multi-column-tuple
branch
from
July 22, 2026 11:31
be36e94 to
5e09063
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
HasSummingMergeTreeEngine("Clicks", "Impressions")generated invalid DDL:ClickHouse rejects this at
CREATE TABLEtime:SummingMergeTree's single optional parameter is the list of columns to sum, which must be a tuple when there is more than one column — unlikeReplacingMergeTree/VersionedCollapsingMergeTree, whose multiple parameters really are positional and correctly comma-separated.Fix
Wrap multiple sum columns in a tuple; pass a single column directly.
Verified empirically against ClickHouse 26.6: the tuple form creates successfully, the comma form fails.
Why it wasn't caught (and how this closes the gap)
Engine DDL was only covered by string-comparison unit tests — and
SummingMergeTree_multiple_columnsactually asserted the invalid SQL as correct. No test ever executed the generatedCREATE TABLEagainst a real server, so the two engines with round-trip integration coverage (MergeTree, ReplacingMergeTree) worked while the rest were unverified.This PR adds engine-argument round-trip integration tests (
EnsureCreatedagainst a real ClickHouse container) for:SummingMergeTree— single and multiple columns (regression guard)CollapsingMergeTreeVersionedCollapsingMergeTreeAggregatingMergeTreeInvalid engine DDL now fails the build. I also audited every other engine's arg generation and verified each form against a live server — SummingMergeTree was the only one affected; the rest emit valid DDL (Graphite only needs a server-side rollup config, which is not a syntax issue).
Tests
All 91 tests in
MigrationIntegrationTests+MigrationSqlGeneratorTestspass.🤖 Generated with Claude Code