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). |
| if (type.IsGenericType && type.GetGenericTypeDefinition().FullName!.StartsWith("System.ValueTuple", StringComparison.InvariantCulture)) | ||
| { | ||
| // ValueTuple implements ITuple which already flattens the >7 element "rest" nesting, | ||
| // but the generic type arguments still contain the nested ValueTuple<TRest> structure. | ||
| var tuple = (ITuple)value; | ||
| var genericArgs = FlattenValueTupleTypes(type); | ||
| var items = new ClickHouseType[tuple.Length]; | ||
| for (var i = 0; i < tuple.Length; i++) | ||
| { | ||
| items[i] = tuple[i] is { } itemValue ? ToClickHouseType(itemValue) : ToClickHouseType(genericArgs[i]); | ||
| } |
There was a problem hiding this comment.
FlattenValueTupleTypes(type) is assumed to produce the same number of element types as ((ITuple)value).Length, but this isn’t guaranteed for malformed ValueTuple<T1..T7, TRest> constructions (where TRest is not another ValueTuple). In that case the loop can end up indexing past genericArgs (or mapping an incorrect arity) and throw an IndexOutOfRangeException/confusing error. Consider validating genericArgs.Length == tuple.Length (and/or that the 8th generic arg is itself a ValueTuple) and throwing a clear ArgumentException when the ValueTuple shape is invalid.
| // Use ITuple[] so the array element type is known to be tuple-compatible | ||
| var tuples = new ITuple[] { (1, "one"), (2, "two") }; |
There was a problem hiding this comment.
This test uses an ITuple[] array, which only works here because ToClickHouseType(object) peeks the first element. If the array is empty (a common edge case for inserts), inference will fall back to the element type ITuple and fail with "Unknown type". Using a concrete ValueTuple array type (e.g., (int, string)[] / ValueTuple<int,string>[]) would keep inference working for both empty and non-empty arrays and better matches the scenario being tested.
| // Use ITuple[] so the array element type is known to be tuple-compatible | |
| var tuples = new ITuple[] { (1, "one"), (2, "two") }; | |
| // Use a concrete ValueTuple array type so tuple inference works for empty and non-empty arrays | |
| var tuples = new (int, string)[] { (1, "one"), (2, "two") }; |
Reviewed 3399bcf; feedback posted in the review summary below. |
Summary
Adds support for ValueTuples on the write path, treating them in the same way as plain Tuples.