ClickHouse / ClickHouse/clickhouse-cs

ReadStringsAsByteArrays breaks GetSchema("Columns") and GetString()

Open
#486 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.