Translate parameterized collection Contains to a native array parameter (#39) - #49
Translate parameterized collection Contains to a native array parameter (#39)#49alex-clickhouse wants to merge 2 commits into
Conversation
…er (#39) A captured collection used with Contains (ids.Contains(x.Id)) now binds as a single native Array(T) parameter — has({ids:Array(T)}, column) — instead of EF Core's default one-scalar-parameter-per-element IN(...) expansion. This removes the parameter-count / query-size ceilings that large IN lists hit and keeps the query text and plan-cache key independent of the collection size. Implemented in ClickHouseSqlNullabilityProcessor.VisitIn: an InExpression with a collection ValuesParameter is rewritten to has() when the parameter is unmarked (provider default) or explicitly EF.Parameter. The array element store type is aligned to the tested column's mapping. Per-query markers are honored — EF.MultipleParameters keeps the per-element expansion and EF.Constant inlines the values. Element types requiring a value conversion (e.g. CLR enum -> Enum8) and collections used as a queryable source (joins, Where(...).Contains(...)) fall back to the existing expansion / SELECT ... UNION ALL ... path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Bumps ClickHouse.Driver 1.1.0 → 1.3.0 (full unit + functional suites green on 1.3.0). 1.3.0 quotes temporal elements inside array parameters correctly, so the parameterized-Contains array path (#39) can now cover more types. Expanded ArrayParameterElementTypes, verified empirically against 1.3.0, to add DateTime/DateTime64, Date/Date32 (DateOnly), Int128/Int256/UInt128/UInt256 (BigInteger), and IPv4/IPv6 (IPAddress). TimeSpan/Time stays excluded — the driver still serializes its array elements unquoted (CANNOT_READ_ARRAY). Added integration tests for the new element types. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR enhances the ClickHouse EF Core provider’s translation of Contains over captured/parameterized primitive collections by binding the collection as a single native Array(T) parameter and rewriting the predicate to has(arrayParam, column), avoiding large IN (p1, …, pN) expansions and parameter-count/query-size limits. It also bumps ClickHouse.Driver to 1.3.0 and updates docs/tests accordingly.
Changes:
- Rewrite
InExpressionwith a collectionValuesParameterintohas({p:Array(T)}, item)inClickHouseSqlNullabilityProcessor, with guarded fallbacks for cases where semantics/serialization could differ. - Add a new integration test suite (
ParameterizedContainsTests) and update existing tests to assert the new SQL shape. - Update documentation (README/CHANGELOG/RELEASENOTES) and bump
ClickHouse.Driverto 1.3.0 across provider and test projects.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.ClickHouse.Tests/ParameterizedContainsTests.cs | New integration coverage for array-parameter Contains translation across types and fallback cases. |
| test/EFCore.ClickHouse.Tests/ExtendedTypeMappingTests.cs | Updates an existing test to assert the new captured-collection array-parameter behavior. |
| test/EFCore.ClickHouse.Tests/EFCore.ClickHouse.Tests.csproj | Bumps ClickHouse.Driver dependency to 1.3.0 for the test project. |
| test/EFCore.ClickHouse.Tests/ClickHouseQuerySqlGeneratorTests.cs | Updates SQL-shape assertion to expect array-parameter Contains translation for captured collections. |
| test/EFCore.ClickHouse.FunctionalTests/EFCore.ClickHouse.FunctionalTests.csproj | Bumps ClickHouse.Driver dependency to 1.3.0 for functional tests. |
| src/EFCore.ClickHouse/Query/Internal/ClickHouseSqlNullabilityProcessor.cs | Implements the IN {collectionParameter} → has(arrayParam, item) rewrite with type-mapping alignment and guarded fallbacks. |
| src/EFCore.ClickHouse/EFCore.ClickHouse.csproj | Bumps provider dependency on ClickHouse.Driver to 1.3.0. |
| RELEASENOTES.md | Documents the new Contains behavior and driver bump for the next release. |
| README.md | Adds user-facing documentation for the new captured-collection Contains translation and opt-outs. |
| CHANGELOG.md | Records the feature and dependency bump with detailed behavior notes and limitations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
I'd probably look to bolt onto IEnumerable<T> rather than T[] as it currently breaks paths that worked prior.
You could achieve this by extending ClickHouseArrayTypeMapping specifically for array parameters (dropping null values and rolling out any lazy enumerables). I've created the changes that may get you a little closer here: https://github.com/DanielBunting/ClickHouse.EntityFrameworkCore/tree/pr-49-review
Note: I think your driver bump may have fixed the HashSet/Lazy cases, but it would still be worth checking with null values in the collection as this could be a silent breakage - an int?[] [1, null, 3] will resolve to [1, 0, 3] and this feels off.
| @@ -0,0 +1,373 @@ | |||
| using Microsoft.EntityFrameworkCore; | |||
There was a problem hiding this comment.
[Query] Is it worth adding support for HashSet/IEnumerable here - quite a common use case is to use the HashSet to remove dupes? Right now it throws with this test:
[Fact]
public async Task HashSet_Collection_Matches()
{
await using var ctx = new ParamContainsDbContext(_fixture.ConnectionString);
var ids = new HashSet<long> { 1L, 3L };
// Driver 1.1.0 cannot serialize HashSet<T> as an array parameter value:
// ArgumentOutOfRangeException "Unknown type: System.Collections.Generic.HashSet`1[System.Int64]".
var query = ctx.Entities.Where(e => ids.Contains(e.Id));
var rows = await query.OrderBy(e => e.Id).Select(e => e.Id).ToListAsync();
Assert.Equal([1L, 3L], rows);
}
There was a problem hiding this comment.
Lazy IEnumerable also throws here, but again - down to if IEnumerable is supported.
| // - the element CLR type isn't one the driver serializes correctly inside an array (see | ||
| // ArrayParameterElementTypes — notably temporal types are excluded). | ||
| // The base expansion serializes each element individually, so all these cases still work. | ||
| if (itemNullable |
There was a problem hiding this comment.
[Query] Nulls inside the collection aren't guarded, and can't be in the processor. Should we strip the nulls at binding time?
Closes #39.
What
A
Containsover a captured collection (ids.Contains(x.Id)) now binds the whole collection as a single nativeArray(T)parameter —has({ids:Array(Int64)}, column)— instead of EF Core's default one-scalar-parameter-per-elementIN (p1, …, pN)expansion.This removes the parameter-count / query-size ceilings that large
INlists hit, and keeps the query text (and plan-cache key) independent of the collection size. ClickHouse is OLAP and doesn't reuse plans by parameterization, so there's no downside to a bound array vs. inlined constants.How
Implemented as an override of
SqlNullabilityProcessor.VisitIninClickHouseSqlNullabilityProcessor. Every route (predicateContains,Any(id => id == x)) converges on anInExpressioncarrying a collectionValuesParameter; that InExpression is rewritten tohas(arrayParam, item)with the array element store type aligned to the tested column's mapping.The rewrite is applied for an unmarked collection (the provider default) or an explicit
EF.Parameter(...). It deliberately falls back to the standard per-element expansion (IN (…)) for:EF.MultipleParameters(...)(keeps one-parameter-per-element) andEF.Constant(...)(inlines literals) — the standard per-query knobs;has(arr, NULL)returns a concrete0, soNOT-Contains would treat NULL rows differently fromx NOT IN (…); deferring to the base path keeps null semantics unchanged;Enum8) — the driver can't serialize the un-converted collection;DateTime,DateTimeOffset,DateOnly,TimeOnly,TimeSpan); the array-parameter path covers integers, floating point,decimal,bool,string, andGuid;Where(…).Contains(…)) — these keep the existingSELECT … UNION ALL …rewrite. The model-wideUseParameterizedCollectionModeknob is intentionally not consulted, since it also governs that path (which doesn't supportParameterTranslationMode.Parameter).Design notes / limitations
http_max_field_value_size(default 128 KiB). This is far better than the parameter-count ceiling and is server-configurable.EF.Parameter(ids)used with a queryable operator (join,.Where().Contains()) still fails to translate — a pre-existing gap that predates this change (it requires overridingTranslatePrimitiveCollectionwith anarrayJoin-style table translation; out of scope here).TranslatePrimitiveCollection(viaarrayJoin) to makeParametermode a first-class native-array citizen everywhere (joins included), which would let the provider defaultParameterizedCollectionModetoParameter.Testing
ParameterizedContainsTests(17 tests): Int32/Int64/String/Guid/List round-trips, element-store-type alignment, empty collection, large (12k) single-parameter,EF.Constant/EF.MultipleParameters/inline behavior, join-still-uses-UNION-ALL, negated Contains, and thedecimal/bool/DateTime/enum/nullable-column fallback matrix.🤖 Generated with Claude Code