Fix reading Enum8/Enum16 columns as integers via DbDataReader

[polyglotAI-bot]

Description

Fixes #439.

Enum8/Enum16 columns are materialized by the binary reader as their string label — EnumType.FrameworkType is typeof(string) and Enum8Type.Read/Enum16Type.Read return Lookup(sbyte/short), which is the label. As a result the numeric DbDataReader accessors did a direct unbox cast on the stored string (e.g. GetInt32 → (int)(object)"Active") and threw InvalidCastException. There was no way to obtain the underlying ordinal without changing the SQL (toInt8(enum_col)).

The fix teaches the integer accessors to recognise an enum column and resolve the stored label back to its underlying ordinal via the enum's existing reverse-lookup map (EnumType.Lookup(string)), leaving the string-returning accessors untouched.

Changes

• ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs:
  • Added a private TryGetEnumOrdinal(...) helper: for a string value whose (Nullable-unwrapped) column type is an EnumType, it returns the ordinal from EnumType.Lookup(label); it returns false for every non-enum column, so their existing cast behaviour is preserved verbatim.
  • GetByte, GetSByte, GetInt16, GetInt32, GetInt64, GetUInt16, GetUInt32, GetUInt64 now return the ordinal for enum columns (via checked Convert.ToXxx), and are unchanged for all other columns.
  • GetFieldValue<T> returns the ordinal for enum columns when T is an integer type (sbyte/byte/short/ushort/int/uint/long/ulong); the IReadValueConverter tail is preserved.
  • GetString, GetValue, and GetFieldValue<string> are unchanged — they still return the enum label (backward compatible).
• CHANGELOG.md / RELEASENOTES.md: added a Bug Fixes entry under Unreleased.
• Regression tests in ClickHouse.Driver.Tests/ADO/DataReaderTests.cs.

Notes:

• The fix covers Enum8, Enum16, and Nullable(Enum...) (a null still surfaces via IsDBNull). LowCardinality(Enum) is intentionally not handled because ClickHouse rejects that type server-side.
• An ordinal that does not fit the requested integer type throws OverflowException rather than truncating silently (e.g. GetByte on an Enum16 ordinal of 1000, or any unsigned accessor on a negative ordinal).

Test

New DB round-trip tests in DataReaderTests, all asserting values derived from the server's own toInt8/16(...) output:

• ShouldReadEnum8ColumnViaNumericAccessors — every integer accessor and GetFieldValue<int/byte/sbyte/long> return the ordinal; GetString/GetValue/GetFieldValue<string> still return the label.
• ShouldReadNegativeEnum8OrdinalAcrossIntegerAccessors — a -1 ordinal returns -1 through the signed accessors; GetByte and the unsigned accessors throw OverflowException (no silent wraparound).
• ShouldReadEnum16OrdinalBeyondByteRangeAcrossIntegerAccessors — a 1000 ordinal returns through the wider signed/unsigned accessors; GetByte/GetSByte throw OverflowException.
• ShouldReadNullableEnum8AsInt — non-null returns the ordinal; null reports IsDBNull.
• ShouldThrowWhenReadingStringColumnAsInt32 — contrast case: a plain String column still throws InvalidCastException, proving the change is scoped to enum columns.

Verified the enum tests fail on main with InvalidCastException and pass with the fix; the full DataReaderTests + EnumTypeTests + ReadValueConverterTests + POCO read tests (132 tests over the touched paths) stay green, with no existing tests modified.

Pre-PR validation gate

• Deterministic repro confirmed (fails on main with InvalidCastException, passes with fix)
• Root cause documented above
• Fix targets the root cause
• Test fails without fix, passes with fix
• No existing tests broken or weakened
• Convention compliance verified per AGENTS.md (test naming, DB round-trip tests, CHANGELOG + RELEASENOTES)
• No public API surface change (additive behaviour on existing accessors)

[github-actions]

Triage

Category: bugfix • Risk: medium

Summary
Fixes issue #439: Enum8/Enum16 columns were always materialized as their string label by the binary reader (EnumType.FrameworkType is typeof(string)), so any call to a numeric accessor (GetByte, GetInt32, GetFieldValue<int>, etc.) threw InvalidCastException. The fix adds EnumType.TryLookup(string, out int) and a TryGetEnumOrdinal helper in ClickHouseDataReader that detects enum-typed columns and resolves the stored label back to its wire ordinal via the enum's value map. All 8 integer accessors and the GetFieldValue<T> generic path are updated; string accessors and IReadValueConverter behavior are unchanged. The binary read path in Enum8Type/Enum16Type itself is not modified. Comprehensive DB round-trip tests cover positive/negative/large ordinals, Nullable(Enum...), and converter interaction.

What this impacts

• ADO/Readers/ClickHouseDataReader.cs — behavioral change: integer accessors now return the ordinal for enum columns instead of throwing InvalidCastException
• Types/EnumType.cs — additive: new TryLookup(string, out int) method
• Tests: DataReaderTests.cs, ReadValueConverterTests.cs, EnumTypeTests.cs
• Docs: CHANGELOG.md, RELEASENOTES.md

Concerns

• Medium rule fired: behavioral change in the ADO/ hot-path module. Previously-throwing accessors now return values for enum columns; no working code is broken (callers relying on the throw would have been workarounds, not intent), but reviewers should confirm the chosen semantic (return wire ordinal, not converter-adjusted ordinal) is intentional and matches user expectations.
• The ordinal lookup bypasses IReadValueConverter — this is intentional and explicitly tested, but is a non-obvious semantic decision (the converter transforms the string label, not the ordinal). Worth a second look from a reviewer familiar with the converter contract.
• FieldValueDispatcher<T>.IsEnumNumericTarget is a static readonly field — no per-call reflection overhead.

Required reviewer action

• At least one human reviewer.

[Copilot]

Pull request overview

This PR fixes a usability gap in the ADO.NET ClickHouseDataReader: Enum8/Enum16 (and Nullable(Enum...)) columns are materialized as their string labels, which previously caused numeric accessors (e.g. GetInt32, GetByte, GetFieldValue<int>) to throw InvalidCastException. The change teaches numeric accessors to recognize enum-typed columns and map the stored label back to its underlying ordinal.

Changes:

• Added enum-aware ordinal resolution in ClickHouseDataReader numeric accessors and GetFieldValue<T> for integer T.
• Added regression tests covering enum ordinals, overflow behavior, nullable enums, and a contrast case for plain strings.
• Documented the fix in CHANGELOG.md and RELEASENOTES.md.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs	Adds enum-aware handling to numeric accessors and GetFieldValue<T> to return underlying enum ordinals instead of failing on string labels.
ClickHouse.Driver.Tests/ADO/DataReaderTests.cs	Adds DB round-trip regression tests validating enum ordinal reads, overflow cases, nullable behavior, and non-enum contrast behavior.
CHANGELOG.md	Notes the enum numeric accessor fix under Unreleased bug fixes.
RELEASENOTES.md	Notes the enum numeric accessor fix under Unreleased bug fixes.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 384b4ed. Configure here.}

[alex-clickhouse]

I'm not super excited about adding this whole rigmarole to every numeric read:

TryGetEnumOrdinal(ordinal, out var ordinalValue)
            ? Convert.ToUInt64(ordinalValue, CultureInfo.InvariantCulture)
            : (ulong)GetValue(ordinal);

What are the performance implications, can you do a quick benchmark? Is there some alternative approach? Maybe we just expose TryGenEnumOrdinal(int ordinal, out value) to the user and lave all the GetInt() etc as they are?

[polyglotAI-bot]

Good call — I benchmarked the per-accessor overhead and then reworked the PR to the shape you suggested.

Benchmark (BenchmarkDotNet, .NET 10, in-memory to isolate just the accessor cost). The enum branch added to each numeric accessor was CurrentRow[ordinal] is string (one object[] load + a type-test that fast-fails on a boxed value type) before falling back to the original cast:

numeric accessor body, non-enum read	mean	ratio
original (int)GetValue(ordinal)	0.68 ns	1.00
with the enum guard	2.71 ns	4.01×

So ~2 ns/call added on every non-enum numeric read. That's negligible against a real read's deserialization + network cost, but you're right that it's a genuine ~4× on the bare accessor and avoidable overhead on a hot path for a niche feature.

So I took your suggestion. GetByte/GetSByte/GetInt16/32/64, GetUInt16/32/64 and GetFieldValue<T> are reverted to their original one-liners — byte-for-byte identical to main, zero overhead — and the enum→ordinal resolution is now an explicit, opt-in accessor:

public bool TryGetEnumOrdinal(int ordinal, out int value)

It returns the underlying wire ordinal for an Enum8/Enum16 (or Nullable(Enum...)) column, and false for a non-enum column, a NULL value, or an unknown label (never throws). The ordinal is the raw wire value, independent of any IReadValueConverter. The standard numeric accessors keep ADO.NET's usual behavior — an enum column is string-backed, so GetInt32 still throws InvalidCastException.

Reworked the tests to target the new API (positive/negative/Enum16-range ordinals, Nullable(Enum), a non-enum/String contrast, and converter-independence), and updated CHANGELOG/RELEASENOTES + the public API surface. Also merged main to clear a CHANGELOG conflict (kept both sides' entries).

Pushed as 60bc85f.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

ClickHouse.Driver/Types/EnumType.cs:62

• EnumType.ToString() currently returns a malformed type string (missing the closing ")"). This leaks into ADO surfaces like ClickHouseDataReader.GetDataTypeName() and can produce confusing output such as Enum(Active=1,Inactive=2.

    public bool TryLookup(string key, out int value) => Values.TryGetValue(key, out value);

    public string Lookup(int value) => reverseValues.TryGetValue(value, out var key) ? key : throw new KeyNotFoundException($"Enum value {value} not found");

    public override string ToString() => $"{Name}({string.Join(",", Values.Select(kvp => kvp.Key + "=" + kvp.Value))}";

ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs:143

• The PR title/description says the numeric DbDataReader accessors (GetInt32, GetByte, GetFieldValue<int>, etc.) were taught to return the enum ordinal, but the implementation and new regression tests keep those accessors throwing InvalidCastException and instead add an opt-in TryGetEnumOrdinal method. Please align the behavior with the PR description/issue (#439) (i.e., make numeric accessors work for enum columns) or update the PR title/description to reflect the opt-in API approach.

    /// <summary>
    /// Attempts to read the underlying integer ordinal of an <c>Enum8</c>/<c>Enum16</c> (or
    /// <c>Nullable(Enum...)</c>) column for the current row. Enum columns materialize as their
    /// string label, so the standard numeric accessors (<c>GetInt32</c>, <c>GetByte</c>, …) throw
    /// <see cref="InvalidCastException"/> on them, matching ADO.NET's behavior for a string-backed
    /// column; use this method to obtain the underlying wire ordinal instead.
    /// </summary>
    /// <param name="ordinal">The zero-based column ordinal.</param>
    /// <param name="value">
    /// When this method returns <see langword="true"/>, the underlying integer ordinal that arrived
    /// on the wire; otherwise <c>0</c>. The ordinal is the raw wire value and is independent of any
    /// configured read value converter (which transforms the string label, not the ordinal).
    /// </param>
    /// <returns>
    /// <see langword="true"/> if the column is an <c>Enum8</c>/<c>Enum16</c> type and the current
    /// value is a known label; otherwise <see langword="false"/> (a non-enum column, a
    /// <see cref="DBNull"/> value, or a value that is not a known enum label).
    /// </returns>
    public bool TryGetEnumOrdinal(int ordinal, out int value)
    {
        if (CurrentRow[ordinal] is string label &&
            GetEffectiveClickHouseType(ordinal) is EnumType enumType &&
            enumType.TryLookup(label, out value))
        {
            return true;
        }

        value = default;
        return false;
    }

[alex-clickhouse]

Note: revisit this after poco box-free reads are merged.