Parameter type resolution extensibility#275
Conversation
…pping Introduces a plugin system that allows users to control the default ClickHouse type inference for @-style parameterized queries. Previously, the only way to override type inference (e.g., DateTime → DateTime64(3)) was to set ClickHouseType on every individual parameter. Key changes: - IParameterTypeResolver interface + DictionaryParameterTypeResolver - Central ParameterTypeResolution class replacing scattered resolution logic - Type resolved exactly once per parameter per request for consistency - ParameterTypeResolver property on ClickHouseClientSettings
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Adds an extensibility point for resolving ClickHouse parameter types for @-style parameters, allowing consumers to override the default CLR→ClickHouse type inference via a new IParameterTypeResolver on ClickHouseClientSettings. This centralizes parameter type resolution into a single flow so SQL placeholder generation and HTTP parameter formatting remain consistent.
Changes:
- Introduces
IParameterTypeResolver(+DictionaryParameterTypeResolver) and wires it intoClickHouseClientSettingsand query execution. - Centralizes type precedence in
ParameterTypeInference(explicit type > SQL hint > resolver > default inference) and updates placeholder replacement / HTTP formatting to use the same resolved type names. - Adds docs/examples and new unit/integration tests validating resolver behavior across HTTP transport modes and ADO.NET.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ClickHouse.Driver/ADO/Parameters/IParameterTypeResolver.cs | New public resolver interface for custom type resolution. |
| ClickHouse.Driver/ADO/Parameters/DictionaryParameterTypeResolver.cs | Simple built-in resolver implementation. |
| ClickHouse.Driver/ADO/Parameters/ParameterTypeInference.cs | Centralized type resolution precedence logic. |
| ClickHouse.Driver/ADO/ClickHouseClientSettings.cs | Adds ParameterTypeResolver setting and copies/compares it. |
| ClickHouse.Driver/ClickHouseClient.cs | Resolves parameter types once per request and reuses for placeholders + HTTP formatting. |
| ClickHouse.Driver/ADO/Parameters/ClickHouseParameterCollection.cs | Replaces @name placeholders using pre-resolved type names. |
| ClickHouse.Driver/Formats/HttpParameterFormatter.cs | Formatting now requires an already-resolved ClickHouse type name. |
| ClickHouse.Driver/ADO/Parameters/ClickHouseDbParameter.cs | QueryForm now uses centralized inference (without resolver/hints). |
| ClickHouse.Driver.Tests/* | Adds/updates tests for resolver precedence and transport consistency. |
| examples/*, CHANGELOG.md, RELEASENOTES.md | Documentation and runnable example for the new resolver feature. |
|
Not really happy with the ParameterTypeInference name, any better ideas would be appreciated. |
|
@codex[agent] review |
Co-authored-by: alex-clickhouse <237136924+alex-clickhouse@users.noreply.github.com>
|
|
Renamed the helper to
Completed review below; no blocking issues found. (a904dfc) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b66b5f97f7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-authored-by: alex-clickhouse <237136924+alex-clickhouse@users.noreply.github.com>
mzitnik
left a comment
mzitnik
left a comment
There was a problem hiding this comment.
LGTM
Just dropped one questions regarding testing
| /// </summary> | ||
| [TestFixture(false)] | ||
| [TestFixture(true)] | ||
| public class ParameterTypeResolutionIntegrationTests |
There was a problem hiding this comment.
General question: Does it make sense to add some unsupported type to the tests
There was a problem hiding this comment.
By unsupported you mean a .NET type that doesn't map to any ClickHouse type, or a mapping that would not actually work? I'd say probably not?
Summary
Currently, when using @-style parameters as opposed to ClickHouse-style {param:type} (eg
SELECT * FROM foo WHERE bar = @param), the driver infers the ClickHouse type based on the .NET type. DateTime maps to DateTime etc. Overriding this requires specifying the type on each parameter, which can be tedious.Here we add a new extensibility point via the
IParameterTypeResolverinterface, a new property on theClickHouseClientSettings. This allows users to customize the parameter type resolution depending on type and/or value. A simple implementation that covers basic use cases,DictionaryParameterTypeResolveris also included.This PR also streamlines parameter type resolution in
ParameterTypeResolution. Previously the logic was scattered across multiple classes; now it's centralized and the priority between different type resolution sources is clear (explicit type > SQL hint > resolver > default inference). Public API listings were updated accordingly, and an advanced example was added to demonstrate resolver usage.Checklist
Delete items not relevant to your PR: