ClickHouse / ClickHouse/ClickHouse.EntityFrameworkCore

Make composite component readers safe for EF Core precompiled queries

Open
#67 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
23
Forks
7
Avg merge
14d 3h
Merged PRs (30d)
1

Description

## Context

PR #63 adds component-by-component materialization for `Array`, `Map`, and `Tuple` mappings. This is required when a component mapping changes the value returned by the driver, including `DateTimeOffset` and `DateOnly`, and for components carrying a `ValueConverter`.

`ClickHouseComponentConversion.CreateConverter()` currently compiles each component reader and embeds the resulting `Func` in the materializer as an `Expression.Constant`. Tuple materialization similarly embeds a `Func[]` constant. This avoids allocating reader delegates for every materialized row, but EF Core cannot render those non-literal constants into generated C#.

This is not currently a user-facing regression: the provider does not yet implement `IQueryCompilationContextFactory.CreatePrecompiled()` or opt into `QueryCompilationContext.SupportsPrecompiledQuery`, so all precompiled queries are provider-wide unsupported today. This issue tracks the composite-materialization part of eventually enabling that feature and should not block #63.

## Reproduction

Temporarily opt the provider into precompiled queries and precompile a query materializing an entity with an `Array(DateTime64(...))` property mapped to `DateTimeOffset[]`:

```shell
dotnet ef dbcontext optimize --precompile-queries --no-scaffold
```

EF Core fails while generating the executor:

```text
Encountered a constant of unsupported type 'Func`2'. Only primitive constant nodes are supported.
System.Func`2[System.Object,System.DateTimeOffset]
```

Affected paths include:

- `ClickHouseArrayTypeMapping` whenever its element needs conversion.
- `ClickHouseMapTypeMapping` whenever its key or value needs conversion.
- `ClickHouseTupleTypeMapping` for converting components, and also for `ValueTuple` materialization even when its components do not otherwise need conversion.
- Nested composites through the same recursive component-reader path.

## Proposed direction

Use an EF liftable constant rather than either a raw delegate constant or an inline per-row lambda:

1. Refactor component-reader construction so the uncompiled `LambdaExpression` remains available.
2. Use the compiled delegate as the runtime value and supply the quotable lambda as the resolver of a `LiftableConstantExpression`.
3. For tuples, lift the complete reader array with a resolver containing `Expression.NewArrayInit(...)` over the component-reader lambdas.
4. Preserve the current fast path and avoid adding delegate or reader-array allocations to the per-row materialization path.

In normal query compilation, EF can resolve the liftable value once while building the query executor. During precompiled-query generation, EF can instead emit a local reader variable initialized from the resolver expression. A prototype of this shape generated a single `Func` local in the executor factory and cleared the unsupported-constant failure.

Do not simply replace `Expression.Constant(delegate)` with an inline `Expression.Lambda`. Under the runtime `Expression.Compile` path, the nested lambda is instantiated for each materializer invocation. A small allocation probe measured approximately 64 additional bytes per row for one reader delegate; a map can require two readers.

## Generated-source accessibility

Lifting the readers exposes a second blocker: generated query code directly calls the composite helpers, but these methods are currently private:

- `ClickHouseArrayTypeMapping.ConvertArray()`
- `ClickHouseMapTypeMapping.ConvertMap()`
- `ClickHouseTupleTypeMapping.ConvertTuple()`

After lifting the DateTimeOffset reader, query generation succeeded but compiling the generated file failed because `ConvertArray` was inaccessible. Replacing it with an equivalent public helper made the generated project build successfully.

Choose one of:

- Expose source-callable public infrastructure helpers, documented as provider internals and hidden from IntelliSense where practical.
- Emit the composite rebuild logic directly in the materializer expression, avoiding calls to non-public provider methods. This avoids public API surface but is substantially more complex, particularly for maps and tuple error handling.

## Value converters and NativeAOT

Built-in `DateTimeOffset`, `DateOnly`, and numeric component readers are naturally quotable because they consist of public static calls and literal parameters. Arbitrary user-defined `ValueConverter` expressions may capture non-literal runtime state; define and test the supported boundary rather than assuming every user converter can be emitted as source.

Precompiled-query generation and NativeAOT should also be treated as separate milestones. `ClickHouseTupleTypeMapping` currently uses `Expression.Compile()` to build its constructor delegate at runtime. That can work for non-AOT precompiled queries, but would need replacement (for example, an emitted `Expression.New`) for full NativeAOT compatibility.

## Provider-wide prerequisites

Enabling precompiled queries for the provider also requires work outside these mappings:

- Implement `CreatePrecompiled(bool async)` in `ClickHouseQueryCompilationContextFactory`.
- Construct the compilation context with `precompiling: true` and override `SupportsPrecompiledQuery`.
- Audit provider-specific SQL expressions, type mappings, and materializer constants for quotability.
- Treat EF Core's precompiled-query APIs as experimental (`EF9100`) and isolate their use accordingly.

## Acceptance criteria

- `dotnet ef dbcontext optimize --precompile-queries` generates code for queries materializing converting `Array`, `Map`, `Tuple`, and nested composite properties.
- The generated source is compiled as part of the test; generation success alone is insufficient.
- Coverage includes `DateTimeOffset`, `DateOnly`, nullable components, and at least one converter-bearing component.
- Generated queries execute against ClickHouse and preserve the existing result semantics.
- Normal runtime queries retain the no-per-row-reader-allocation behavior.
- Any unsupported captured `ValueConverter` shape fails with a clear diagnostic.
- NativeAOT support is either tested end to end or explicitly kept as a separate follow-up.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.