Add IReadValueConverter#293
Open
alex-clickhouse wants to merge 5 commits intomainfrom
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an IReadValueConverter hook to allow same-CLR-type transformations of values returned from ClickHouseDataReader, configurable globally via ClickHouseClientSettings and overridable per-query via QueryOptions.
Changes:
- Introduces
IReadValueConverterand threads it throughClickHouseClient.ExecuteReaderAsync/ ADO.NET reader creation. - Applies conversion on
GetValue(),GetFieldValue<T>(), andGetValues()while preserving existing metadata APIs. - Adds documentation (changelog/release notes), an advanced example, benchmarks, and NUnit coverage for the new behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| examples/README.md | Adds the new advanced example to the examples index. |
| examples/Program.cs | Runs the new ReadValueConverter example in the examples harness. |
| examples/Advanced/Advanced_013_ReadValueConverter.cs | Demonstrates global/per-query converters and ADO.NET behavior. |
| RELEASENOTES.md | Documents the new IReadValueConverter feature for the release. |
| CHANGELOG.md | Adds the new feature entry to the changelog. |
| ClickHouse.Driver/QueryOptions.cs | Adds QueryOptions.ReadValueConverter for per-query override. |
| ClickHouse.Driver/ClickHouseClient.cs | Passes the effective converter into reader creation. |
| ClickHouse.Driver/ADO/Readers/IReadValueConverter.cs | Defines the new converter interface. |
| ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs | Applies the converter in GetValue, GetValues, and GetFieldValue<T> and captures raw type strings when needed. |
| ClickHouse.Driver/ADO/ClickHouseCommand.cs | Passes the configured converter through the ADO.NET command path. |
| ClickHouse.Driver/ADO/ClickHouseClientSettings.cs | Adds ClickHouseClientSettings.ReadValueConverter and includes it in copy/equality/hash. |
| ClickHouse.Driver.Tests/ADO/ReadValueConverterTests.cs | Adds NUnit tests covering conversion behavior, overrides, and forwarded type strings. |
| ClickHouse.Driver.Benchmark/ReadValueBenchmark.cs | Adds BenchmarkDotNet benchmarks measuring converter overhead. |
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.
Summary
IReadValueConverterinterface for same-type transformation of values returned byClickHouseDataReader(e.g.,DateTime.SpecifyKindfor UTC, string trimming/normalization).ClickHouseClientSettings.ReadValueConverteror per-query viaQueryOptions.ReadValueConverter.GetValue()(boxed path) andGetFieldValue<T>()(generic path). Zero overhead when no converter is set.GetFieldType,GetSchemaTable) is not rerouted through the converter and must stay consistent.One important consideration for the future: the GetFieldValue path, despite being generic, currently still boxes every value, which has a big perf cost. We are planning to change that in the future by adding
Read<T>()methods to all the Type classes. The current design should be compatible with that future change and avoid any additional perf penalties.Performance
Benchmarked on 500K rows across Int32/DateTime/String with GetValue and GetFieldValue.
Users who don't set a converter pay nothing; users who do pay only for the conversion work itself.
Alternative Approach
Also considered going for per-column/per-type config instead, something like:
This would have some benefits:
Downside is that it would be quite a bit more complex to set up. And some new questions come up, like what if multiple converters match?
Going with the one-converter approach also doesn't stop us from also adding the per-column converter in the future. But that might be a bit much.
Thoughts?