diff --git a/CHANGELOG.md b/CHANGELOG.md index dff11e1..3fb786b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ v0.3.1 (Unreleased) --- ### Bug fixes * `Sum`/`SumAsync` over a `double` or `float` column no longer throws `InvalidCastException`. EF Core wraps a top-level aggregate so the empty case returns `0`, supplying that fallback as a boxed `Int32` carrying the `Float64`/`Float32` mapping; the literal generators now convert rather than unbox. The `Float32` read path also converts, since ClickHouse widens `sum(Float32)` to `Float64` (which the driver's `GetFloat()` refuses to downcast). ([#46](https://github.com/ClickHouse/ClickHouse.EntityFrameworkCore/issues/46)) +* **SummingMergeTree with multiple sum columns**: `HasSummingMergeTreeEngine("A", "B")` now generates valid DDL (`SummingMergeTree((A, B))`). Previously it emitted a comma-separated argument list (`SummingMergeTree(A, B)`), which ClickHouse rejects with `NUMBER_OF_ARGUMENTS_DOESNT_MATCH`. Single-column and no-column usage are unaffected. v0.3.0 --- diff --git a/RELEASENOTES.md b/RELEASENOTES.md index dd56054..e0043a5 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -2,6 +2,7 @@ v0.3.1 (Unreleased) --- ### Bug fixes * Summing a `double` or `float` column (`.SumAsync(x => x.Value)`) no longer throws `InvalidCastException`. Two ClickHouse-specific mismatches were biting: EF Core hands the float literal generator a boxed `Int32` `0` as the empty-result fallback, and ClickHouse widens `sum(Float32)` to `Float64` so the driver couldn't read it back as a `float`. Both the literal generation and the `Float32` read path now convert instead of hard-casting. ([#46](https://github.com/ClickHouse/ClickHouse.EntityFrameworkCore/issues/46)) (Thanks to @HotTotem!) +* **SummingMergeTree with multiple sum columns** now produces valid DDL. Configuring more than one sum column (`HasSummingMergeTreeEngine("A", "B")`) previously emitted `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. v0.3.0 --- diff --git a/src/EFCore.ClickHouse/Migrations/ClickHouseMigrationsSqlGenerator.cs b/src/EFCore.ClickHouse/Migrations/ClickHouseMigrationsSqlGenerator.cs index 4f26041..fe7c523 100644 --- a/src/EFCore.ClickHouse/Migrations/ClickHouseMigrationsSqlGenerator.cs +++ b/src/EFCore.ClickHouse/Migrations/ClickHouseMigrationsSqlGenerator.cs @@ -538,7 +538,15 @@ private void GenerateEngineArgs(CreateTableOperation operation, string engine, M case ClickHouseAnnotationNames.SummingMergeTree: var columns = (string[]?)operation.FindAnnotation(ClickHouseAnnotationNames.SummingMergeTreeColumns)?.Value; if (columns is { Length: > 0 }) - builder.Append(string.Join(", ", columns.Select(QuoteColumnOrExpression))); + { + var quotedColumns = columns.Select(QuoteColumnOrExpression); + // SummingMergeTree accepts a single optional parameter: the column to sum, or a + // tuple of columns. Multiple columns must be wrapped in a tuple — emitting them as + // a comma-separated argument list produces invalid DDL (NUMBER_OF_ARGUMENTS_DOESNT_MATCH). + builder.Append(columns.Length == 1 + ? quotedColumns.First() + : $"({string.Join(", ", quotedColumns)})"); + } break; case ClickHouseAnnotationNames.CollapsingMergeTree: diff --git a/test/EFCore.ClickHouse.Tests/MigrationIntegrationTests.cs b/test/EFCore.ClickHouse.Tests/MigrationIntegrationTests.cs index 3293eb3..a4a41ec 100644 --- a/test/EFCore.ClickHouse.Tests/MigrationIntegrationTests.cs +++ b/test/EFCore.ClickHouse.Tests/MigrationIntegrationTests.cs @@ -659,6 +659,133 @@ public async Task Column_with_default_value_applied_by_clickhouse_on_insert() Assert.NotEqual("1970-01-01 00:00:00", createdAt); // not epoch — got now() } + // ── Engine-argument round-trips (execute CREATE TABLE against real server) ── + // These guard against generating engine DDL that our string-based unit tests accept + // but ClickHouse rejects. EnsureCreatedAsync throws if the CREATE TABLE is invalid, + // so reaching the assertions proves the generated DDL actually ran on the server. + + [Fact] + public async Task SummingMergeTree_single_column_creates_valid_table() + { + await using var ctx = CreateContext(b => + { + b.Entity(e => + { + e.HasKey(x => x.Id); + e.ToTable("smt_single_test", t => t + .HasSummingMergeTreeEngine("Amount") + .WithOrderBy("Id")); + }); + }); + await ctx.Database.EnsureDeletedAsync(); + await ctx.Database.EnsureCreatedAsync(); + + var engine = await QueryScalar(ctx, + $"SELECT engine FROM system.tables WHERE database = '{_databaseName}' AND name = 'smt_single_test'"); + Assert.Equal("SummingMergeTree", engine); + + // ClickHouse normalizes the stored engine expression without backticks. + var createSql = await QueryScalar(ctx, + $"SELECT create_table_query FROM system.tables WHERE database = '{_databaseName}' AND name = 'smt_single_test'"); + Assert.Contains("SummingMergeTree(Amount)", createSql!); + } + + [Fact] + public async Task SummingMergeTree_multiple_columns_creates_valid_table() + { + // Regression: multiple sum columns must be emitted as a tuple, e.g. + // SummingMergeTree((`Amount`, `Count`)). The comma-separated form + // SummingMergeTree(`Amount`, `Count`) fails with NUMBER_OF_ARGUMENTS_DOESNT_MATCH, + // so EnsureCreatedAsync would throw before reaching the assertions below. + await using var ctx = CreateContext(b => + { + b.Entity(e => + { + e.HasKey(x => x.Id); + e.ToTable("smt_multi_test", t => t + .HasSummingMergeTreeEngine("Amount", "Count") + .WithOrderBy("Id")); + }); + }); + await ctx.Database.EnsureDeletedAsync(); + await ctx.Database.EnsureCreatedAsync(); + + var engine = await QueryScalar(ctx, + $"SELECT engine FROM system.tables WHERE database = '{_databaseName}' AND name = 'smt_multi_test'"); + Assert.Equal("SummingMergeTree", engine); + + // The stored DDL must carry the sum columns as a tuple (ClickHouse normalizes + // away the backticks). The comma-separated form never reaches this point. + var createSql = await QueryScalar(ctx, + $"SELECT create_table_query FROM system.tables WHERE database = '{_databaseName}' AND name = 'smt_multi_test'"); + Assert.Contains("SummingMergeTree((Amount, Count))", createSql!); + } + + [Fact] + public async Task CollapsingMergeTree_creates_valid_table() + { + await using var ctx = CreateContext(b => + { + b.Entity(e => + { + e.HasKey(x => x.Id); + e.Property(x => x.Sign).HasColumnType("Int8"); + e.ToTable("cmt_test", t => t + .HasCollapsingMergeTreeEngine("Sign") + .WithOrderBy("Id")); + }); + }); + await ctx.Database.EnsureDeletedAsync(); + await ctx.Database.EnsureCreatedAsync(); + + var engine = await QueryScalar(ctx, + $"SELECT engine FROM system.tables WHERE database = '{_databaseName}' AND name = 'cmt_test'"); + Assert.Equal("CollapsingMergeTree", engine); + } + + [Fact] + public async Task VersionedCollapsingMergeTree_creates_valid_table() + { + await using var ctx = CreateContext(b => + { + b.Entity(e => + { + e.HasKey(x => x.Id); + e.Property(x => x.Sign).HasColumnType("Int8"); + e.ToTable("vcmt_test", t => t + .HasVersionedCollapsingMergeTreeEngine("Sign", "Version") + .WithOrderBy("Id")); + }); + }); + await ctx.Database.EnsureDeletedAsync(); + await ctx.Database.EnsureCreatedAsync(); + + var engine = await QueryScalar(ctx, + $"SELECT engine FROM system.tables WHERE database = '{_databaseName}' AND name = 'vcmt_test'"); + Assert.Equal("VersionedCollapsingMergeTree", engine); + } + + [Fact] + public async Task AggregatingMergeTree_creates_valid_table() + { + await using var ctx = CreateContext(b => + { + b.Entity(e => + { + e.HasKey(x => x.Id); + e.ToTable("amt_test", t => t + .HasAggregatingMergeTreeEngine() + .WithOrderBy("Id")); + }); + }); + await ctx.Database.EnsureDeletedAsync(); + await ctx.Database.EnsureCreatedAsync(); + + var engine = await QueryScalar(ctx, + $"SELECT engine FROM system.tables WHERE database = '{_databaseName}' AND name = 'amt_test'"); + Assert.Equal("AggregatingMergeTree", engine); + } + // ── Helpers ────────────────────────────────────────────────────────────── private TestContext CreateContext(Action configure) @@ -766,4 +893,18 @@ public class FullFeaturedEntity public string Name { get; set; } = string.Empty; public DateTime CreatedAt { get; set; } } + + public class SummingEntity + { + public long Id { get; set; } + public long Amount { get; set; } + public long Count { get; set; } + } + + public class CollapsingEntity + { + public long Id { get; set; } + public sbyte Sign { get; set; } + public ulong Version { get; set; } + } } diff --git a/test/EFCore.ClickHouse.Tests/MigrationSqlGeneratorTests.cs b/test/EFCore.ClickHouse.Tests/MigrationSqlGeneratorTests.cs index 1f69333..398d99b 100644 --- a/test/EFCore.ClickHouse.Tests/MigrationSqlGeneratorTests.cs +++ b/test/EFCore.ClickHouse.Tests/MigrationSqlGeneratorTests.cs @@ -587,7 +587,25 @@ public void SummingMergeTree_multiple_columns() op.Columns.Add(new AddColumnOperation { Name = "Id", ColumnType = "Int64", ClrType = typeof(long) }); }); - Assert.Contains("ENGINE = SummingMergeTree(`Amount`, `Count`)", sql); + // Multiple sum columns must be wrapped in a tuple. `SummingMergeTree(`Amount`, `Count`)` + // is invalid DDL — ClickHouse rejects it with NUMBER_OF_ARGUMENTS_DOESNT_MATCH. + Assert.Contains("ENGINE = SummingMergeTree((`Amount`, `Count`))", sql); + } + + [Fact] + public void SummingMergeTree_single_column() + { + var sql = GenerateCreateTable(op => + { + op.AddAnnotation(ClickHouseAnnotationNames.Engine, ClickHouseAnnotationNames.SummingMergeTree); + op.AddAnnotation(ClickHouseAnnotationNames.SummingMergeTreeColumns, new[] { "Amount" }); + op.AddAnnotation(ClickHouseAnnotationNames.OrderBy, new[] { "Id" }); + op.Columns.Add(new AddColumnOperation { Name = "Id", ColumnType = "Int64", ClrType = typeof(long) }); + }); + + // A single sum column is passed directly, without tuple wrapping. + Assert.Contains("ENGINE = SummingMergeTree(`Amount`)", sql); + Assert.DoesNotContain("SummingMergeTree((", sql); } [Fact]