ClickHouse / ClickHouse/clickhouse-cs
ReadStringsAsByteArrays breaks GetSchema("Columns") and GetString()
- Dominant language
- C#
- Stars
- 94
- Forks
- 22
- Avg merge
- 11h 26m
- Merged PRs (30d)
- 22
Description
Two independent bugs with a common cause, filed together because the fix likely wants to be one decision. Both reproduce on `main` @ `02ce9d2`, verified at runtime against a 25.10 server, and neither is covered by any existing test.
When `ReadStringsAsByteArrays = true`, `StringType`/`FixedStringType` return `byte[]` instead of `string`. Anything downstream that assumes `string` breaks — silently or loudly depending on the site.
Found while fixing the same class of bug in the JSON decoder (#485). Filing separately rather than widening that PR, since #2 below needs a design decision that shouldn't be bundled with a JSON fix.
---
## 1. `GetSchema("Columns")` throws `InvalidCastException`
```
ReadStringsAsByteArrays = true -> InvalidCastException: Unable to cast object of
type 'System.Byte[]' to type 'System.String'.
ReadStringsAsByteArrays = false -> 1 row(s), ProviderType=UInt8
```
Repro:
```csharp
var settings = new ClickHouseClientSettings { /* host etc. */, ReadStringsAsByteArrays = true };
using var conn = new ClickHouseConnection(settings);
await conn.OpenAsync();
conn.GetSchema("Columns", new[] { "system", "one", null }); // throws
```
`Utility/SchemaDescriber.cs:109`:
```csharp
var clickHouseType = TypeConverter.ParseClickHouseType((string)row["ProviderType"], TypeSettings.Default);
row["ProviderType"] = clickHouseType.ToString();
```
`DescribeColumns` runs a real query — `SELECT database as Database, table as Table, name as Name, type as ProviderType, type as DataType FROM system.columns` (`SchemaDescriber.cs:85`) — through `ClickHouseDataAdapter.Fill`. The reader is built with the connection's own `TypeSettings` (`ADO/ClickHouseCommand.cs:210` → `ClickHouseClient.cs:174`), which carries the flag. All five projected columns are `String`, so every cell in the filled `DataTable` is a `byte[]` and the cast on line 109 fails.
**There is a second failure hiding behind the first.** The `DataColumn` CLR types come from `GetFieldType` → `FrameworkType` → `byte[]`. So fixing only the cast on 109 moves the exception to 110, where a `string` is written back into a `byte[]`-typed column (`ArgumentException`). Both need handling.
Suggested fix: have `SchemaDescriber` build its reader with `readStringsAsByteArrays = false`. This is the driver querying `system.columns` for its own metadata, not returning user data — that metadata is always text, and overriding at the source fixes the `DataColumn` types too rather than patching two call sites. (`Utility/SchemaResolver.cs:92-95`, the other internal query, reads header metadata only via `WHERE 1=0` and never touches a value, so it is unaffected.)
`ClickHouse.Driver.Tests/ADO/ConnectionTests.cs:314,322` already call `GetSchema("Columns", …)`, but only with default settings.
---
## 2. `GetString()` returns the literal string `"System.Byte[]"`
```
ReadStringsAsByteArrays = true -> GetValue(0).GetType() = Byte[]
GetString(0) = "System.Byte[]"
ReadStringsAsByteArrays = false -> GetValue(0).GetType() = String
GetString(0) = "hello"
```
Repro: `SELECT 'hello'::String`, then `reader.GetString(0)`.
`ADO/Readers/ClickHouseDataReader.cs:188`:
```csharp
public override string GetString(int ordinal) => GetValue(ordinal)?.ToString();
```
`object.ToString()` on an array yields its type name. No exception, every row, and the result is plausible enough to travel a long way before anyone notices. This is the same shape as the JSON bug in #485 — a lenient catch-all converting a `byte[]` into convincing garbage.
**The right semantic here is genuinely debatable, and it is not the same as the JSON case.** #485 resolved its version by always UTF-8-decoding, justified by RFC 8259: a string *inside a JSON document* is text by definition. That reasoning does not transfer. `GetString` on a `String` column is precisely the case this flag exists for — the bytes may not be valid UTF-8, and lenient decoding would substitute U+FFFD for real data.
- **(a) UTF-8 decode.** Consistent with `GetString`'s existing leniency and with flag-off behaviour, making `GetString` flag-independent. But it silently transforms bytes the caller explicitly asked to receive raw.
- **(b) Throw `InvalidCastException`.** Matches the reader's other typed accessors (`GetByte`/`GetGuid`/`GetDecimal`, `:135-171`), which already throw on a type mismatch. Tells the caller to use `GetValue`/`GetFieldValue`.
- **(c) Strict decode** (`UTF8Encoding(throwOnInvalidBytes: true)`) — decode when it is unambiguously text, throw when it is not.
I lean (b), as the only option that never silently transforms data, and the flag is opt-in so anyone setting it knows they are handling bytes. Happy to implement whichever you prefer.
---
## Related, same family
`Formats/HttpParameterFormatter.cs:92-103` has a `byte[]` arm for `FixedStringType` but not for `StringType`, so a `byte[]` bound to a `{x:String}` placeholder formats as `value.ToString()` → `"System.Byte[]"`. That is the write-path mirror of #2 and completes a read-then-filter round trip, so it probably wants the same decision.
Minor: `Types/BinaryTypeDecoder.cs:330-340` `DecodeCustomType` ignores `typeSettings` and falls back to `new StringType()`, so it is a third flag construction site that does not honour the flag. No impact today (Custom types are the geo set, `Float64` leaves only) — noting it for completeness.
## Checked and clean
For whoever picks this up: the composite types are safe by construction. `Array`/`Map`/`Tuple`/`Nested`/`Nullable`/`LowCardinality`/`Variant`/`Dynamic`/`QBit` all derive element types from `UnderlyingType.FrameworkType`, which is already `byte[]` under the flag, so they stay internally consistent. Geo types have `Float64` leaves only. POCO materialization fails fast with a descriptive `InvalidOperationException` from `ValidateBinding` before any row is materialized, so a `byte[]` never silently reaches a `string` setter. `EnumType` reads an integer and returns a name, never touching String decode.
I'm happy to open PRs for either or both — just let me know your preference on the #2 semantics first, since that determines the shape.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Contributor guide
Research direction
Start with Utility/SchemaDescriber.cs:85-110 and ADO/ClickHouseCommand.cs:210, then reproduce the flagged GetSchema failure using the settings in the issue. Review ADO/Readers/ClickHouseDataReader.cs:188 and the typed accessors at :135-171, plus Formats/HttpParameterFormatter.cs:92-103. Add regression coverage alongside ADO/ConnectionTests.cs:314,322; done means both metadata reads and GetString have an explicit, agreed result under ReadStringsAsByteArrays.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- database
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100