Conversation
|
@codex[agent] review |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Pull request overview
Adds write-path support for System.ValueTuple by treating ValueTuples the same as tuples during ClickHouse type inference and parameter/binary insert handling, including flattening of compiler-generated TRest nesting for tuples with >7 elements.
Changes:
- Extend
TypeConverterto recognizeSystem.ValueTuple(type-based and value-based inference) and flattenTRestnesting for >7 elements. - Add/extend tests covering ValueTuple type mapping, binary inserts, and SQL parameterized queries.
- Document the new feature in release notes and changelog.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| RELEASENOTES.md | Document ValueTuple write-path support for v1.3.0. |
| CHANGELOG.md | Add v1.3.0 entry describing ValueTuple write-path support. |
| ClickHouse.Driver/Types/TypeConverter.cs | Add ValueTuple detection and TRest-flattening for accurate ClickHouse Tuple(...) inference. |
| ClickHouse.Driver.Tests/Types/TypeMappingTests.cs | Add type mapping and value-based inference test cases for ValueTuple, including >7 elements. |
| ClickHouse.Driver.Tests/Types/TupleTypeTests.cs | Add binary insert round-trip tests for ValueTuple scenarios (nested, nullable, arrays, >7 elements). |
| ClickHouse.Driver.Tests/SQL/SqlParameterizedSelectTests.cs | Add SQL parameterization tests passing ValueTuple parameters (including nested). |
Reviewed 3399bcf; feedback posted in the review summary below. |
Summary
InsertBinaryAsync, mapping them to ClickHouse Tuple(T1, T2, ...) columnsTupleType.Write()already handled both via the ITuple interface, so the changes are in type inference (TypeConverter)<T1, T2, ..., T7, TRest>) is another ValueTuple that has the rest of the elements. We handle this scenario by flattening the structure.Known limitation
When a ValueTuple has exactly 7 scalar elements followed by a tuple element (e.g., Tuple(Int32, ..., Int32, Tuple(String, String))), the driver cannot distinguish this from a 9-element flat tuple. Both produce the same .NET type due to C# compiler TRest nesting. The driver treats the 8th generic argument as TRest (flattens it). The workaround is a doubly-nested tuple for the 8th element. Will also add a note for this in the docs.