From 1e669985a026f874299db28fe9dd77ebf8536b17 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 27 Aug 2026 20:46:10 +0000 Subject: [PATCH 01/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index 5f994f4b..49f1ab0f 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -110,3 +110,9 @@ The E2E suite runs on whichever protocol the run configures (Thrift by default i **Rule:** On the SEA path, `IArrowArrayStream` is a strongly-typed contract where the declared manifest schema and each batch's column arrays MUST agree on type — so whenever `ArrowTypeParser` maps a SQL type to an Arrow type that differs from what the IPC data delivers (e.g. VOID→Utf8 but bytes are a `NullArray`), you must also insert a serializing stream that rewrites the batch arrays to the declared type; changing only the type mapping produces a self-inconsistent schema-vs-batch result. - **Context:** In run #31539108512 the author guessed the C# test project path (`dotnet test csharp/test/Apache.Arrow.Adbc.Tests.Drivers.Databricks.csproj`), which failed, then had to glob `csharp/test/*.csproj` and retry with the actual name before tests would run. **Rule:** The csharp test project in this repo is `csharp/test/AdbcDrivers.Databricks.Tests.csproj` — glob `csharp/test/*.csproj` to confirm the path before invoking `dotnet test`, rather than guessing an Apache.Arrow-style csproj name and wasting a build/test cycle. + +### 2026-08-27: learnings since 2026-08-26T18:05:02Z +- **Context:** PR #645 fixed GetCrossReference over-return on the SEA path by filtering the SHOW FOREIGN KEYS rows against the caller's requested parent identifiers, mirroring the JDBC reference driver's CrossReferenceKeysDatabricksResultSetAdapter.includeRow. + **Rule:** SHOW FOREIGN KEYS is scoped to the FOREIGN table only, so it returns FKs to EVERY parent — when the caller specifies parent catalog/schema/table you must filter the returned rows down to that parent client-side (case-insensitive equality; a null requested field means "no constraint" so the GetColumnsExtended foreign-only reuse passing null must stay unfiltered). Genuine FK rows always populate the parent identifier columns (JDBC's includeRow calls equalsIgnoreCase directly on them and would NPE otherwise), so treating a null/empty server parent value as a non-match is safe and correctly collapses the empty-string-parent case to zero rows. +- **Context:** PR #645 review (F1): the cross-reference parent filter initially compared against the value-population fallback (`srv ?? requested ?? ""`), so for any row whose server parent column was null it compared the requested value against itself — silently turning the filter into a no-op and re-admitting rows for the wrong parent. + **Rule:** When a field is BOTH a filter key and populated with a fallback to the requested/input value, capture the raw source value FIRST and filter on that raw value; apply the requested-value fallback only afterward for row population. Filtering on the fallback compares the requested value against itself and disables the filter for exactly the rows where the source is absent/null. From 9c3d3819e0b6b1b3a2434515ff860ae2385895ea Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 28 Aug 2026 21:07:14 +0000 Subject: [PATCH 02/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index 49f1ab0f..b36b9f8c 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -116,3 +116,11 @@ The E2E suite runs on whichever protocol the run configures (Thrift by default i **Rule:** SHOW FOREIGN KEYS is scoped to the FOREIGN table only, so it returns FKs to EVERY parent — when the caller specifies parent catalog/schema/table you must filter the returned rows down to that parent client-side (case-insensitive equality; a null requested field means "no constraint" so the GetColumnsExtended foreign-only reuse passing null must stay unfiltered). Genuine FK rows always populate the parent identifier columns (JDBC's includeRow calls equalsIgnoreCase directly on them and would NPE otherwise), so treating a null/empty server parent value as a non-match is safe and correctly collapses the empty-string-parent case to zero rows. - **Context:** PR #645 review (F1): the cross-reference parent filter initially compared against the value-population fallback (`srv ?? requested ?? ""`), so for any row whose server parent column was null it compared the requested value against itself — silently turning the filter into a no-op and re-admitting rows for the wrong parent. **Rule:** When a field is BOTH a filter key and populated with a fallback to the requested/input value, capture the raw source value FIRST and filter on that raw value; apply the requested-value fallback only afterward for row population. Filtering on the fallback compares the requested value against itself and disables the filter for exactly the rows where the source is absent/null. + +### 2026-08-28: learnings since 2026-08-27T20:44:51Z +- **Context:** PR #656 moved the server feature-flag fetch from a background `Task.Run` to a synchronous blocking fetch on `Connect()`, initially serialized behind a single process-wide `SemaphoreSlim` on the `FeatureFlagCache` singleton; a reviewer caught the latency implication. + **Rule:** When a per-connect network fetch is serialized behind a single process-wide lock, concurrent first-time (cold-cache) connects to DIFFERENT hosts queue behind one another and worst-case connect latency grows ~N×fetch-timeout — not the flat single-timeout a "bounded by the fetch timeout" comment implies. Key the create/dedup lock per-host (e.g. `ConcurrentDictionary`) so different cold hosts fetch in parallel, while same-host concurrent connects still dedup to one fetch via a post-lock double-check. +- **Context:** PR #656 added a lazily `GetOrAdd`'d per-host `SemaphoreSlim` map on the process-lifetime `FeatureFlagCache` singleton; a reviewer flagged it had no removal path (leaking one lock per host talked to), fixed by removing the lock in the cache eviction callback. + **Rule:** A per-key lock map kept alongside a cache must drop its entry in the cache's eviction callback (not only in `Dispose`), or it grows unbounded over a long-lived process talking to many keys. On eviction, `TryRemove` the lock reference rather than `Dispose()`-ing it: an in-flight holder may still be inside `WaitAsync`/`Release`, and disposing out from under them throws `ObjectDisposedException`. A `SemaphoreSlim` whose `AvailableWaitHandle` was never accessed holds no OS handle, so dropping the reference leaks nothing; a racing create may briefly re-add a fresh lock, weakening dedup for one window but never correctness. +- **Context:** PR #654 wrapped the Thrift `GetTables` result stream (`TableTypeDefaultingStream`) to substitute `"TABLE"` for any null/empty `TABLE_TYPE`, matching databricks-jdbc `MetadataResultSetBuilder`. + **Rule:** `GetTables` must not return rows with an empty/null `TABLE_TYPE` — the Thrift path leaves some tables (e.g. legacy `hive_metastore` tables in a broad enumeration) with an empty type, so default any empty/null server-supplied `TABLE_TYPE` to `"TABLE"` for JDBC parity (bluntly, like databricks-jdbc: even a view with an empty server type becomes `"TABLE"`; non-empty values pass through). Implement by wrapping the result stream and rewriting only the plain `StringArray` encoding (fail-open / no-op on other encodings), and guard a null `result.Stream` and pass it through, matching the sibling `MaybeWrapComplexTypes` wrapper. From 514804326f40f36b5db142cfd8fef93e7d5ac14f Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Tue, 1 Sep 2026 17:28:26 +0000 Subject: [PATCH 03/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index b36b9f8c..7fe6a5eb 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -124,3 +124,7 @@ The E2E suite runs on whichever protocol the run configures (Thrift by default i **Rule:** A per-key lock map kept alongside a cache must drop its entry in the cache's eviction callback (not only in `Dispose`), or it grows unbounded over a long-lived process talking to many keys. On eviction, `TryRemove` the lock reference rather than `Dispose()`-ing it: an in-flight holder may still be inside `WaitAsync`/`Release`, and disposing out from under them throws `ObjectDisposedException`. A `SemaphoreSlim` whose `AvailableWaitHandle` was never accessed holds no OS handle, so dropping the reference leaks nothing; a racing create may briefly re-add a fresh lock, weakening dedup for one window but never correctness. - **Context:** PR #654 wrapped the Thrift `GetTables` result stream (`TableTypeDefaultingStream`) to substitute `"TABLE"` for any null/empty `TABLE_TYPE`, matching databricks-jdbc `MetadataResultSetBuilder`. **Rule:** `GetTables` must not return rows with an empty/null `TABLE_TYPE` — the Thrift path leaves some tables (e.g. legacy `hive_metastore` tables in a broad enumeration) with an empty type, so default any empty/null server-supplied `TABLE_TYPE` to `"TABLE"` for JDBC parity (bluntly, like databricks-jdbc: even a view with an empty server type becomes `"TABLE"`; non-empty values pass through). Implement by wrapping the result stream and rewriting only the plain `StringArray` encoding (fail-open / no-op on other encodings), and guard a null `result.Stream` and pass it through, matching the sibling `MaybeWrapComplexTypes` wrapper. + +### 2026-09-01: learnings since 2026-08-31T17:26:42Z +- **Context:** PR #660 (csharp SEA/StatementExecution GetCrossReference) — `_metadataCatalogName` is seeded from the connection's default catalog at construction, so passing it as the parent-catalog filter in a getImportedKeys-style call (no parent supplied) wrongly dropped every FK whose real parent lives in another catalog; SEA returned empty while Thrift returned the row. + **Rule:** Fields seeded from the connection's default catalog (e.g. `_metadataCatalogName`) are NOT a reliable "caller supplied this argument" signal — track an explicit "was set" flag (set only in SetOption/CatalogName) and pass the parent catalog into cross-reference/metadata filters only when the caller explicitly set it, else null; unseeded fields like parent schema/table already no-op when unset. Regression fixtures must use a parent catalog DIFFERENT from the seeded default (e.g. seed "main", FK parent "prod_cat"), because a fixture whose parent catalog equals the seed makes the seeded value happen to match and masks the bug. From 312e460be43690005b36126f00a3d403ffac316e Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 4 Sep 2026 17:28:03 +0000 Subject: [PATCH 04/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index 7fe6a5eb..b381881f 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -128,3 +128,11 @@ The E2E suite runs on whichever protocol the run configures (Thrift by default i ### 2026-09-01: learnings since 2026-08-31T17:26:42Z - **Context:** PR #660 (csharp SEA/StatementExecution GetCrossReference) — `_metadataCatalogName` is seeded from the connection's default catalog at construction, so passing it as the parent-catalog filter in a getImportedKeys-style call (no parent supplied) wrongly dropped every FK whose real parent lives in another catalog; SEA returned empty while Thrift returned the row. **Rule:** Fields seeded from the connection's default catalog (e.g. `_metadataCatalogName`) are NOT a reliable "caller supplied this argument" signal — track an explicit "was set" flag (set only in SetOption/CatalogName) and pass the parent catalog into cross-reference/metadata filters only when the caller explicitly set it, else null; unseeded fields like parent schema/table already no-op when unset. Regression fixtures must use a parent catalog DIFFERENT from the seeded default (e.g. seed "main", FK parent "prod_cat"), because a fixture whose parent catalog equals the seed makes the seeded value happen to match and masks the bug. + +### 2026-09-04: learnings since 2026-09-03T17:28:33Z +- **Context:** PR #662 de-flaked C# E2E MAP-column assertions (`ComplexTypesValueTests` / `DatabricksTestEnvironment.NormalizeMapJson`) that had compared serialized MAPs by exact key order. + **Rule:** Databricks `MAP` key order is unspecified server-side and the JDBC reference preserves that server order without sorting, so compare a serialized MAP's content order-insensitively — normalize by sorting only the top-level object's keys. Keep the normalization SHALLOW: preserve each value's raw JSON verbatim so nested STRUCT field order (which IS significant) is untouched; a MAP nested inside a value re-introduces the key-order flake and would need recursive normalization. +- **Context:** PR #509 rewrote `ComplexTypeSerializingStream` to a manual `Utf8JsonWriter` for AOT/trim safety; a reviewer caught that `WriteRawValue` was used for Time32/Time64 values formatted as `HH:mm:ss.ffffff`. + **Rule:** `Utf8JsonWriter.WriteRawValue` requires a syntactically valid JSON token (number/string/bool/null/object/array) and throws `JsonException` at runtime for anything else — an unquoted time/date string with colons is NOT valid JSON. Use `WriteStringValue` for values that must be quoted strings and reserve `WriteRawValue` for raw numeric text (e.g. full-precision DECIMAL). +- **Context:** PR #509 replaced reflection-based `JsonSerializer.Serialize(object)` with a hand-written explicit type-switch (`SerializeStructuredValue`) and a hand-written telemetry payload writer, to make the driver trim-/NativeAOT-safe under net10.0 (`IsAotCompatible`). + **Rule:** Replacing a reflection serializer with a hand-written one silently changes semantics: the switch's `default` arm mis-categorizes any unenumerated type (e.g. emitting a number as a JSON string), and a new property on a model is silently omitted with no test failure — behaviors the reflection path handled automatically. Defend the rewrite with a round-trip test asserting hand-written output equals the reflection-based output for a fully-populated instance (keep the reflection options around for tests), and/or a `Debug.Assert(false, ...)` in the default arm so DEBUG unit tests catch newly-added types before they ship. From 7888768d44054369ff19c3b86e18385949ae751c Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Wed, 9 Sep 2026 17:30:15 +0000 Subject: [PATCH 05/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index b381881f..85e00a77 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -136,3 +136,15 @@ The E2E suite runs on whichever protocol the run configures (Thrift by default i **Rule:** `Utf8JsonWriter.WriteRawValue` requires a syntactically valid JSON token (number/string/bool/null/object/array) and throws `JsonException` at runtime for anything else — an unquoted time/date string with colons is NOT valid JSON. Use `WriteStringValue` for values that must be quoted strings and reserve `WriteRawValue` for raw numeric text (e.g. full-precision DECIMAL). - **Context:** PR #509 replaced reflection-based `JsonSerializer.Serialize(object)` with a hand-written explicit type-switch (`SerializeStructuredValue`) and a hand-written telemetry payload writer, to make the driver trim-/NativeAOT-safe under net10.0 (`IsAotCompatible`). **Rule:** Replacing a reflection serializer with a hand-written one silently changes semantics: the switch's `default` arm mis-categorizes any unenumerated type (e.g. emitting a number as a JSON string), and a new property on a model is silently omitted with no test failure — behaviors the reflection path handled automatically. Defend the rewrite with a round-trip test asserting hand-written output equals the reflection-based output for a fully-populated instance (keep the reflection options around for tests), and/or a `Debug.Assert(false, ...)` in the default arm so DEBUG unit tests catch newly-added types before they ship. + +### 2026-09-09: learnings since 2026-09-08T17:27:54Z +- **Context:** PR #670 added a process-wide `static` ReydenWarehouseCache; a reviewer caught it was keyed on the bare warehouse id. + **Rule:** Databricks SQL warehouse ids are only workspace-unique (small zero-padded counters, not globally-unique GUIDs), so any process-wide/static per-warehouse cache MUST be keyed on a `(host, warehouseId)` composite — a bare-id key lets one workspace's mark leak to a same-id warehouse in another workspace (silent protocol/behavior switch for the TTL). Lowercase the host component to match sibling caches (FeatureFlagCache) so mixed-casing hosts don't split into distinct entries. +- **Context:** PR #670's `Connect` constructs a connection then calls `OpenAsync().Wait()` / `ApplyServerSidePropertiesAsync().Wait()`; a reviewer flagged that a partial open (OpenAsync succeeds, ApplyServerSideProperties throws) leaks the connection's HTTP client / handler chain — worse now that a fallback opens a second connection. + **Rule:** When a factory constructs-then-opens a resource that owns an HTTP client / handler chain (especially when a fallback may open another), wrap the open in `try { open(); return conn; } catch { conn.Dispose(); throw; }` so a partial or failed open disposes it before propagating — and apply the dispose-on-failure guard symmetrically to EVERY open path, not just the one that triggers the fallback. +- **Context:** PR #670's transparent Thrift→SEA fallback originally discarded the original Thrift rejection when the SEA fallback also threw, leaving it recoverable only from an untraced Activity event. + **Rule:** When a transparent fallback (e.g. a protocol downgrade) also fails, throw an AggregateException retaining BOTH the original error and the fallback error so the causal chain survives outside of tracing — never let the primary attempt's exception be silently dropped. Order the inner exceptions deliberately so the existing AggregateException→AdbcException unwrap (a depth-first search for the first AdbcException) surfaces the intended primary. +- **Context:** PR #670's transparent Thrift→SEA fallback initially emitted no telemetry, so an operator who set `protocol=thrift` and observed a SEA connection had no signal why the protocol changed. + **Rule:** A transparent/silent protocol or behavior downgrade must emit a trace/telemetry event (matching the codebase's existing `Activity.Current?.AddEvent(...)` convention) at BOTH the pre-check short-circuit and the catch-and-retry, tagged with the identifying key, or the switch is undiagnosable in the field. Emit discrete tag dimensions (e.g. host, warehouse_id) rather than a composite key that embeds a separator like '\n' — some OpenTelemetry exporters/log sinks mangle or truncate embedded newlines/control chars. +- **Context:** PR #670 made the driver transparently fall back to SEA when a Reyden / Lakehouse-RT warehouse rejects a Thrift `OpenSession`. + **Rule:** Reyden / Lakehouse-RT SQL warehouses do not support Thrift and reject `OpenSession` with SQLSTATE `KP001` (surfaced on `HiveServer2Exception.SqlState`); detect it by walking the whole exception chain INCLUDING `AggregateException.InnerExceptions`, and cache the rejection per `(host, warehouseId)` with a bounded self-healing TTL so later connects skip the doomed handshake — the Thrift `OpenSession` otherwise retries across protocol versions (V10→V7) before it fails. From 2daf2874f2313c27c54e47d7cf5b589735f1dff0 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 17 Sep 2026 17:29:09 +0000 Subject: [PATCH 06/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index 85e00a77..cc965028 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -148,3 +148,7 @@ The E2E suite runs on whichever protocol the run configures (Thrift by default i **Rule:** A transparent/silent protocol or behavior downgrade must emit a trace/telemetry event (matching the codebase's existing `Activity.Current?.AddEvent(...)` convention) at BOTH the pre-check short-circuit and the catch-and-retry, tagged with the identifying key, or the switch is undiagnosable in the field. Emit discrete tag dimensions (e.g. host, warehouse_id) rather than a composite key that embeds a separator like '\n' — some OpenTelemetry exporters/log sinks mangle or truncate embedded newlines/control chars. - **Context:** PR #670 made the driver transparently fall back to SEA when a Reyden / Lakehouse-RT warehouse rejects a Thrift `OpenSession`. **Rule:** Reyden / Lakehouse-RT SQL warehouses do not support Thrift and reject `OpenSession` with SQLSTATE `KP001` (surfaced on `HiveServer2Exception.SqlState`); detect it by walking the whole exception chain INCLUDING `AggregateException.InnerExceptions`, and cache the rejection per `(host, warehouseId)` with a bounded self-healing TTL so later connects skip the doomed handshake — the Thrift `OpenSession` otherwise retries across protocol versions (V10→V7) before it fails. + +### 2026-09-17: learnings since 2026-09-16T17:28:25Z +- **Context:** In run #35240939263 (adding named-parameter support to the Databricks C# driver) the agent spent ~15 turns hunting for the base `AdbcStatement.Bind` signature — globbing for `AdbcStatement.cs`, `find csharp/arrow-adbc`, repeated `git diff`/`git submodule status` on `csharp/arrow-adbc` — before finally reading it via WebFetch from `raw.githubusercontent.com/apache/arrow-adbc/apache-arrow-adbc-18/...`. + **Rule:** The ADBC base classes (`AdbcStatement`, `AdbcConnection`, etc.) come from the `Apache.Arrow.Adbc` NuGet package pinned in `csharp/Directory.Packages.props`; their source is NOT in this repo and there is no checked-out `csharp/arrow-adbc` submodule and no `context_repo` for it (unlike the vendored `hiveserver2` base). To find a base-class signature like `AdbcStatement.Bind`, read the pinned version from `Directory.Packages.props` and fetch the source from `apache/arrow-adbc` at the matching `apache-arrow-adbc-` tag — don't glob/find/git-diff for a local copy that doesn't exist. From ea80e4fda2a4a85ebfee9e8b341e693e8d5a0963 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Mon, 21 Sep 2026 17:29:47 +0000 Subject: [PATCH 07/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index cc965028..880ba567 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -152,3 +152,13 @@ The E2E suite runs on whichever protocol the run configures (Thrift by default i ### 2026-09-17: learnings since 2026-09-16T17:28:25Z - **Context:** In run #35240939263 (adding named-parameter support to the Databricks C# driver) the agent spent ~15 turns hunting for the base `AdbcStatement.Bind` signature — globbing for `AdbcStatement.cs`, `find csharp/arrow-adbc`, repeated `git diff`/`git submodule status` on `csharp/arrow-adbc` — before finally reading it via WebFetch from `raw.githubusercontent.com/apache/arrow-adbc/apache-arrow-adbc-18/...`. **Rule:** The ADBC base classes (`AdbcStatement`, `AdbcConnection`, etc.) come from the `Apache.Arrow.Adbc` NuGet package pinned in `csharp/Directory.Packages.props`; their source is NOT in this repo and there is no checked-out `csharp/arrow-adbc` submodule and no `context_repo` for it (unlike the vendored `hiveserver2` base). To find a base-class signature like `AdbcStatement.Bind`, read the pinned version from `Directory.Packages.props` and fetch the source from `apache/arrow-adbc` at the matching `apache-arrow-adbc-` tag — don't glob/find/git-diff for a local copy that doesn't exist. + +### 2026-09-21: learnings since 2026-09-20T17:27:11Z +- **Context:** PR #684 rewrote `SetDefaultCatalogAndSchemaOptionsTest` to stop hard-coding `hive_metastore`/`main` as the expected default catalog, discovering it live via `SELECT current_catalog()` (GetWarehouseDefaultCatalogAsync) in the same multi-catalog mode as the case. + **Rule:** The Databricks workspace default catalog (what `current_catalog()` returns when the driver pins nothing) is not stable — it drifted from `hive_metastore` to `main` and also depends on multiple-catalog mode (Thrift legacy/off yields hive_metastore, UC/on yields the workspace default). Never hard-code it as a test literal; discover it live in the same mode as the case under test and derive expectations from that value. +- **Context:** PR #684 replaced `StatementCatalogNotScopedWhenFlagOff`'s assertion (previously expecting a `TABLE_OR_VIEW_NOT_FOUND`/42P01 resolution failure) with a direct check that `current_catalog()` stays on the session default when the flag is off, targeting a catalog chosen to differ from that default. + **Rule:** To test whether a statement catalog target is/isn't scoped, assert the observable session state (`current_catalog()` unchanged vs. changed) against a target picked to differ from the discovered session default — do not rely on a name-resolution failure like TABLE_OR_VIEW_NOT_FOUND, which depends on warehouse quirks (cross-catalog fallback) and on which catalog happens to be default, making the test fragile as the workspace default drifts. +- **Context:** In PR #684 a reviewer caught that the new `GetWarehouseDefaultCatalogAsync` helper leaked ~12 live warehouse sessions because it opened connections via the `NewConnection(testConfig)` overload without `using`. + **Rule:** `TestBase.NewConnection(testConfig)` returns a brand-new AdbcConnection that the base class does NOT track — `Dispose` only tears down the lazily-created `_connection`/`_statement` singletons, so any connection (and its statements/streams) created through the testConfig overload is the caller's responsibility and must be wrapped in `using`; this matters in parameterized theories where the helper runs once per case. Also note xUnit constructs a fresh test-class instance per theory case, so caching an invariant discovered value must use a `static` field, not an instance field. +- **Context:** PR #681 pipelined SEA GetResultChunk link prefetches and added observation for an abandoned in-flight `_prefetchTask`; the review thread debated at length whether the cancellation/StopAsync loop-boundary path could leak an unobserved task exception, resolved (and verified empirically on net472) by noting that path leaves the task Canceled. + **Rule:** In .NET, `TaskScheduler.UnobservedTaskException` fires only for **Faulted** tasks, never **Canceled** ones — so an abandoned look-ahead/prefetch task tied to a cancelled token completes Canceled and needs no observation; only the non-cancellation fault path (e.g. the task started before a mid-enqueue error that exits the loop without cancelling the token) can leave an unobserved fault and must be observed (attach a fault-reading continuation). Wire that observation into the path that actually runs in production (here the FetchNextBatchAsync catch / loop-exit finally), not a Dispose that a single-use object routed through a non-IDisposable interface never invokes. From af508a80570178beca085d2026a15fb288a39fd3 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 25 Sep 2026 17:30:44 +0000 Subject: [PATCH 08/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index 880ba567..aea1ee95 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -162,3 +162,9 @@ The E2E suite runs on whichever protocol the run configures (Thrift by default i **Rule:** `TestBase.NewConnection(testConfig)` returns a brand-new AdbcConnection that the base class does NOT track — `Dispose` only tears down the lazily-created `_connection`/`_statement` singletons, so any connection (and its statements/streams) created through the testConfig overload is the caller's responsibility and must be wrapped in `using`; this matters in parameterized theories where the helper runs once per case. Also note xUnit constructs a fresh test-class instance per theory case, so caching an invariant discovered value must use a `static` field, not an instance field. - **Context:** PR #681 pipelined SEA GetResultChunk link prefetches and added observation for an abandoned in-flight `_prefetchTask`; the review thread debated at length whether the cancellation/StopAsync loop-boundary path could leak an unobserved task exception, resolved (and verified empirically on net472) by noting that path leaves the task Canceled. **Rule:** In .NET, `TaskScheduler.UnobservedTaskException` fires only for **Faulted** tasks, never **Canceled** ones — so an abandoned look-ahead/prefetch task tied to a cancelled token completes Canceled and needs no observation; only the non-cancellation fault path (e.g. the task started before a mid-enqueue error that exits the loop without cancelling the token) can leave an unobserved fault and must be observed (attach a fault-reading continuation). Wire that observation into the path that actually runs in production (here the FetchNextBatchAsync catch / loop-exit finally), not a Dispose that a single-use object routed through a non-IDisposable interface never invokes. + +### 2026-09-25: learnings since 2026-09-24T17:28:48Z +- **Context:** PR #689 tightened `QueryTimeoutSeconds` validation to accept any non-negative `int` (up to `int.MaxValue`), but reviewers noted the value is later fed to `new CancellationTokenSource(TimeSpan.FromSeconds(n))` (StatementExecutionStatement.cs:518, `CreateMetadataTimeoutCts`, and the Thrift `ThriftResultFetcher` path), which throws when total milliseconds exceed `int.MaxValue` (~2147483 s / ~24.8 days). + **Rule:** When validating a numeric timeout/duration option, bound the accepted range to what the DOWNSTREAM consumer accepts, not just the parse type's range — a value that passes `int.TryParse`/`>=0` still overflows `CancellationTokenSource(TimeSpan.FromSeconds(n))` (requires total ms ≤ int.MaxValue) and defers the failure to execution time; declaring a value "valid" that reliably throws when used is a validation gap. +- **Context:** PR #689 made SEA's `StatementExecutionStatement.SetOption(QueryTimeoutSeconds, ...)` call `ParseNonNegativeIntPropertyWithValidation` purely for its throw-on-invalid side effect, then `break` — but `_queryTimeoutSeconds` is a `readonly` field set only in the constructor, so a valid runtime `SetOption` is silently a no-op on SEA while the Thrift base (`HiveServer2Statement.SetOption`) mutates and applies it. The re-enabled shared conformance test `ValidateCanSetOptionQueryTimeout` asserts only throw/no-throw and never checks the value is honored, so it passes over the divergence. + **Rule:** A shared conformance/option test that asserts only that a `SetOption` value throws-or-not does NOT verify the option takes effect — a path that validates-but-discards (e.g. writing to a readonly, construction-time field) passes it while silently diverging from the path that actually applies the value; when a setter can only validate (not apply) on one protocol, either store+honor it to match the sibling path or document the construction-time-only limitation, and add an assertion that the option is actually honored rather than relying on the throw/no-throw check. From 3c1fb94c7121e9cd321fef2972401d8e35b46d85 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Tue, 6 Oct 2026 17:31:50 +0000 Subject: [PATCH 09/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index aea1ee95..0a521f9d 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -168,3 +168,11 @@ The E2E suite runs on whichever protocol the run configures (Thrift by default i **Rule:** When validating a numeric timeout/duration option, bound the accepted range to what the DOWNSTREAM consumer accepts, not just the parse type's range — a value that passes `int.TryParse`/`>=0` still overflows `CancellationTokenSource(TimeSpan.FromSeconds(n))` (requires total ms ≤ int.MaxValue) and defers the failure to execution time; declaring a value "valid" that reliably throws when used is a validation gap. - **Context:** PR #689 made SEA's `StatementExecutionStatement.SetOption(QueryTimeoutSeconds, ...)` call `ParseNonNegativeIntPropertyWithValidation` purely for its throw-on-invalid side effect, then `break` — but `_queryTimeoutSeconds` is a `readonly` field set only in the constructor, so a valid runtime `SetOption` is silently a no-op on SEA while the Thrift base (`HiveServer2Statement.SetOption`) mutates and applies it. The re-enabled shared conformance test `ValidateCanSetOptionQueryTimeout` asserts only throw/no-throw and never checks the value is honored, so it passes over the divergence. **Rule:** A shared conformance/option test that asserts only that a `SetOption` value throws-or-not does NOT verify the option takes effect — a path that validates-but-discards (e.g. writing to a readonly, construction-time field) passes it while silently diverging from the path that actually applies the value; when a setter can only validate (not apply) on one protocol, either store+honor it to match the sibling path or document the construction-time-only limitation, and add an assertion that the option is actually honored rather than relying on the throw/no-throw check. + +### 2026-10-06: learnings since 2026-10-05T17:31:31Z +- **Context:** PR #693 switched the SEA/StatementExecution client from `ReadAsStringAsync()` + `JsonSerializer.Deserialize(string)` to `ReadAsByteArrayAsync()` + `Deserialize(ReadOnlySpan)` for a perf win, and a reviewer flagged a behavioral asymmetry at the BOM/charset edge. + **Rule:** The `JsonSerializer.Deserialize` byte/`ReadOnlySpan` overload assumes UTF-8 and does NOT strip a leading UTF-8 BOM (0xEF,0xBB,0xBF), unlike the `Stream`/`DeserializeAsync` overloads and unlike `ReadAsStringAsync()` which strips the BOM and honors the `Content-Type` charset during decode — so swapping string-deserialize for bytes-deserialize silently changes behavior at that edge (a BOM-prefixed or non-UTF-8 body that parsed before now throws `JsonException`); only use the bytes overload when the source is guaranteed raw UTF-8 with no BOM, or skip a leading BOM defensively first. +- **Context:** PR #678 added `SeaResponseCompressionEnabled`, a strictly-typed boolean connection parameter read on the connect path with the throwing `PropertyHelper.GetBooleanPropertyWithValidation`, but a reviewer caught it was missing the `[FeatureFlagType(FeatureFlagValueKind.Boolean)]` attribute every sibling boolean param carries. + **Rule:** `FeatureFlagValueValidator` builds its drop-malformed-server-value registry by reflecting over constants that carry `[FeatureFlagType(...)]`, and unknown (unregistered) keys pass through `IsAcceptable` unconditionally — so any strictly-typed (non-string) connection parameter that is both read on the connect path with a throwing `GetXxxPropertyWithValidation` getter AND can be pushed by the connector service MUST carry the `[FeatureFlagType]` attribute, or a malformed server value (e.g. literal `"null"`) survives the validator, reaches the throwing getter, and breaks connection setup for every client of that workspace; add a `FeatureFlagValueValidatorTests` case asserting the key validates and drops bad values. +- **Context:** PR #678 disabled gzip on the SEA statements client by setting the handler's `AutomaticDecompression = DecompressionMethods.None`; reviewers warned that an absent `Accept-Encoding` lets the server pick any coding while the handler no longer decompresses, and the fix was to send `Accept-Encoding: identity` explicitly (verified end-to-end against a dogfood warehouse). + **Rule:** In .NET, setting `HttpClientHandler.AutomaticDecompression = None` removes BOTH the auto-added `Accept-Encoding` request header and the decode path — and you cannot keep decompression enabled while suppressing the header, because any non-`None` `AutomaticDecompression` re-adds `gzip`/`deflate` to `Accept-Encoding`. A request with no `Accept-Encoding` means "no preference" (RFC 9110 §12.5.3), so the server may still return a compressed body that the handler won't decode, handing raw gzip bytes to the JSON parser; when disabling automatic decompression, explicitly send `Accept-Encoding: identity` to request an uncompressed body, and scope the toggle to the one client that needs it (don't disable compression on the shared OAuth/auth handler, whose tiny token responses gain nothing). From 1a7309f34fa4d332a5708af27207dff80be623bd Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 9 Oct 2026 17:32:13 +0000 Subject: [PATCH 10/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index 0a521f9d..45538912 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -176,3 +176,13 @@ The E2E suite runs on whichever protocol the run configures (Thrift by default i **Rule:** `FeatureFlagValueValidator` builds its drop-malformed-server-value registry by reflecting over constants that carry `[FeatureFlagType(...)]`, and unknown (unregistered) keys pass through `IsAcceptable` unconditionally — so any strictly-typed (non-string) connection parameter that is both read on the connect path with a throwing `GetXxxPropertyWithValidation` getter AND can be pushed by the connector service MUST carry the `[FeatureFlagType]` attribute, or a malformed server value (e.g. literal `"null"`) survives the validator, reaches the throwing getter, and breaks connection setup for every client of that workspace; add a `FeatureFlagValueValidatorTests` case asserting the key validates and drops bad values. - **Context:** PR #678 disabled gzip on the SEA statements client by setting the handler's `AutomaticDecompression = DecompressionMethods.None`; reviewers warned that an absent `Accept-Encoding` lets the server pick any coding while the handler no longer decompresses, and the fix was to send `Accept-Encoding: identity` explicitly (verified end-to-end against a dogfood warehouse). **Rule:** In .NET, setting `HttpClientHandler.AutomaticDecompression = None` removes BOTH the auto-added `Accept-Encoding` request header and the decode path — and you cannot keep decompression enabled while suppressing the header, because any non-`None` `AutomaticDecompression` re-adds `gzip`/`deflate` to `Accept-Encoding`. A request with no `Accept-Encoding` means "no preference" (RFC 9110 §12.5.3), so the server may still return a compressed body that the handler won't decode, handing raw gzip bytes to the JSON parser; when disabling automatic decompression, explicitly send `Accept-Encoding: identity` to request an uncompressed body, and scope the toggle to the one client that needs it (don't disable compression on the shared OAuth/auth handler, whose tiny token responses gain nothing). + +### 2026-10-09: learnings since 2026-10-08T17:31:37Z +- **Context:** PR #686 (Go driver named parameters) guards its lazy multi-row result concatenator with `arrow.Schema.Equal`, which a reviewer flagged as too strict for parameterized `SELECT`s that project a parameter marker (e.g. `SELECT :id`). + **Rule:** `arrow.Schema.Equal` compares field **names and metadata**, not just column count and types — so it rejects per-row result schemas that differ only by column name (an unaliased projected parameter marker gets named after the substituted literal) or by type (an untyped NULL rendered as VOID vs a typed value on other rows). When checking that per-batch/per-row schemas are structurally compatible, compare column count + field types explicitly rather than relying on `Schema.Equal`, and emit typed nulls (keyed off the column's Arrow type) so projected-parameter result schemas stay stable across rows. +- **Context:** PR #686's SQL marker-scanner (`parameterMarkers`) applied C-style backslash escaping uniformly across single-quote, double-quote, and backtick states, which a reviewer caught mishandles backtick identifiers. + **Rule:** In Databricks/Spark SQL, backtick-quoted identifiers are NOT backslash-escaped — an embedded backtick is escaped by doubling it (`` `` ``) and a lone backslash is a literal character; backslash-as-escape applies only to single-quoted (and, under default config, double-quoted) string literals. A SQL tokenizer that treats `\` as an escape inside backtick identifiers mis-scans identifiers ending in `\` and can swallow a following parameter marker, corrupting the positional-vs-named classification; scope backslash escaping to the string-literal quote kinds only. +- **Context:** PR #686 enabled native Arrow decimals (issue #106) via the `dbsql.WithArrowNativeDecimal(true)` connector option, but a reviewer flagged that the URI/DSN connection path ignores connector options entirely, so the fix had to be re-applied by injecting a `useArrowNativeDecimal=true` DSN query parameter. + **Rule:** In the Databricks Go driver, `initializeConnectionPool` uses connector options (`dbsql.WithX`) only on the non-URI path; when `d.uri != ""` it opens the DSN string and the connector options are dropped. Any connection-level behavior fix must be applied to BOTH paths (connector option AND DSN query-string key), and the DSN-key assembly test is not sufficient — add end-to-end coverage proving the driver version actually honors the DSN key, since a DSN that merely parses can still silently no-op the setting. +- **Context:** PR #686 serializes Arrow `TIMESTAMP` parameters via `GetToTimeFunc` + RFC3339 formatting; a reviewer noted the only test covered a `TimeZone: "UTC"` column, leaving the timezone-naive case unexercised. + **Rule:** For an Arrow `TimestampType` with an empty `TimeZone` (naive / Databricks `TIMESTAMP_NTZ`), `GetToTimeFunc` resolves the zone to UTC, so formatting with `time.RFC3339Nano` attaches a `Z` offset and binds a wall-clock value as a UTC instant — shifting it by the session offset if the caller meant a session-local value. Arrow naive timestamps are inherently ambiguous: decide and document whether they are treated as UTC (and test that case explicitly), don't let the UTC relabeling happen implicitly. From a75dc8f7dcd30e9dd3c6c9a6703591144b8c191f Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Tue, 13 Oct 2026 17:33:26 +0000 Subject: [PATCH 11/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index 45538912..2bf5a934 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -186,3 +186,9 @@ The E2E suite runs on whichever protocol the run configures (Thrift by default i **Rule:** In the Databricks Go driver, `initializeConnectionPool` uses connector options (`dbsql.WithX`) only on the non-URI path; when `d.uri != ""` it opens the DSN string and the connector options are dropped. Any connection-level behavior fix must be applied to BOTH paths (connector option AND DSN query-string key), and the DSN-key assembly test is not sufficient — add end-to-end coverage proving the driver version actually honors the DSN key, since a DSN that merely parses can still silently no-op the setting. - **Context:** PR #686 serializes Arrow `TIMESTAMP` parameters via `GetToTimeFunc` + RFC3339 formatting; a reviewer noted the only test covered a `TimeZone: "UTC"` column, leaving the timezone-naive case unexercised. **Rule:** For an Arrow `TimestampType` with an empty `TimeZone` (naive / Databricks `TIMESTAMP_NTZ`), `GetToTimeFunc` resolves the zone to UTC, so formatting with `time.RFC3339Nano` attaches a `Z` offset and binds a wall-clock value as a UTC instant — shifting it by the session offset if the caller meant a session-local value. Arrow naive timestamps are inherently ambiguous: decide and document whether they are treated as UTC (and test that case explicitly), don't let the UTC relabeling happen implicitly. + +### 2026-10-13: learnings since 2026-10-12T17:33:31Z +- **Context:** PR #692 (csharp SEA/StatementExecution) reworked the per-catalog metadata fanout in `ReadColumnsAsync` and the all-catalogs GetColumns path; reviewers repeatedly flagged the bare `catch { continue; }` (commented as skipping per-catalog permission errors) now also swallowing `OperationCanceledException`/`TimeoutException`. + **Rule:** In a per-catalog (or per-object) metadata fanout loop, a bare `catch { continue; }` meant to skip inaccessible catalogs also eats cancellation and the metadata-timeout CTS — when the token fires mid-fanout every remaining iteration throws instantly and is swallowed, so the loop runs to exhaustion and returns a silently-truncated result instead of surfacing the timeout (the caller's `when (cts.IsCancellationRequested)` guard never sees it). Narrow the catch with a `when` filter (e.g. `ex.IsObjectNotFoundException()`, matching the sibling `PopulateColumnInfoAsync`) so only permission/not-found is skipped while `OperationCanceledException`/`TimeoutException` and genuine validation errors propagate. +- **Context:** PR #692 added native SEA metadata results; reviewers found `GetTableSchema` passed neither `requireExactCatalog` nor pattern-matching (so it merged columns from sibling catalogs for a name like `my_catalog`), and that GetObjects compared a catalog *pattern* (`ma%`) with exact equality, dropping all native tables (regression of 3d7ea2a, demonstrated by `ColumnFanout_FiltersOverlappingNativeCatalogNames`). + **Rule:** On the native SEA metadata path the server treats a catalog scope (`IN CATALOG \`cat\``) as a LIKE pattern, so native result rows can include sibling catalogs and must be filtered client-side by the right predicate: flat `GetTables`/`GetColumns`/`GetTableSchema` use EXACT match (`requireExactCatalog`/`MatchesCatalog`), while `GetObjects` uses PATTERN match (`MatchesCatalogPattern`). Mixing them silently breaks results in opposite directions — comparing a pattern exactly drops every row, and leaving flat scopes unfiltered merges same-named tables from sibling catalogs; model the catalog intent as one explicit exact-vs-pattern value rather than spreading it across several flags/helpers. From 47eee9180feda4e53348f1ebe18528b8f665e985 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Tue, 20 Oct 2026 17:31:29 +0000 Subject: [PATCH 12/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index 2bf5a934..726472fb 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -192,3 +192,9 @@ The E2E suite runs on whichever protocol the run configures (Thrift by default i **Rule:** In a per-catalog (or per-object) metadata fanout loop, a bare `catch { continue; }` meant to skip inaccessible catalogs also eats cancellation and the metadata-timeout CTS — when the token fires mid-fanout every remaining iteration throws instantly and is swallowed, so the loop runs to exhaustion and returns a silently-truncated result instead of surfacing the timeout (the caller's `when (cts.IsCancellationRequested)` guard never sees it). Narrow the catch with a `when` filter (e.g. `ex.IsObjectNotFoundException()`, matching the sibling `PopulateColumnInfoAsync`) so only permission/not-found is skipped while `OperationCanceledException`/`TimeoutException` and genuine validation errors propagate. - **Context:** PR #692 added native SEA metadata results; reviewers found `GetTableSchema` passed neither `requireExactCatalog` nor pattern-matching (so it merged columns from sibling catalogs for a name like `my_catalog`), and that GetObjects compared a catalog *pattern* (`ma%`) with exact equality, dropping all native tables (regression of 3d7ea2a, demonstrated by `ColumnFanout_FiltersOverlappingNativeCatalogNames`). **Rule:** On the native SEA metadata path the server treats a catalog scope (`IN CATALOG \`cat\``) as a LIKE pattern, so native result rows can include sibling catalogs and must be filtered client-side by the right predicate: flat `GetTables`/`GetColumns`/`GetTableSchema` use EXACT match (`requireExactCatalog`/`MatchesCatalog`), while `GetObjects` uses PATTERN match (`MatchesCatalogPattern`). Mixing them silently breaks results in opposite directions — comparing a pattern exactly drops every row, and leaving flat scopes unfiltered merges same-named tables from sibling catalogs; model the catalog intent as one explicit exact-vs-pattern value rather than spreading it across several flags/helpers. + +### 2026-10-20: learnings since 2026-10-19T17:30:43Z +- **Context:** PR #699 (csharp geospatial Arrow types) mapped Databricks GEOMETRY/GEOGRAPHY into a GeoArrow WKB extension field; a reviewer questioned whether EWKB is valid in a `geoarrow.wkb` column and the author confirmed the spec/reader behavior. + **Rule:** For geospatial columns with a fixed SRID, emit ISO WKB; for ANY-SRID (`-1` sentinel, per-row varying SRID) ISO WKB cannot preserve each row's SRID, so EWKB is the only lossless single-column representation — but a `geoarrow.wkb`-labeled column carrying EWKB is a known spec deviation: geoarrow-c and geopandas accept EWKB while GDAL rejects it, so treat ANY-SRID EWKB as a documented deviation (readers may reject it or silently drop the embedded SRID) rather than assuming GeoArrow reader support. +- **Context:** PR #699 reviewers flagged that `TryCreate("GEOGRAPHY(3857)")` initially succeeded; the author added rejection + tests so GEOGRAPHY accepts only SRID 4326 or ANY. + **Rule:** Databricks GEOGRAPHY supports only type-level SRID 4326 (WGS84) or ANY (`-1`) — any other fixed SRID (0, 3857, etc.) is invalid and must be rejected as malformed, whereas GEOMETRY accepts any nonnegative SRID or ANY; enforce and test this asymmetry at both the SQL-type-parse and the Arrow-tag-resolve boundaries. From ab063462a53c00835e371b836a938862422d792f Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Wed, 21 Oct 2026 17:33:14 +0000 Subject: [PATCH 13/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index 726472fb..da174548 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -198,3 +198,17 @@ The E2E suite runs on whichever protocol the run configures (Thrift by default i **Rule:** For geospatial columns with a fixed SRID, emit ISO WKB; for ANY-SRID (`-1` sentinel, per-row varying SRID) ISO WKB cannot preserve each row's SRID, so EWKB is the only lossless single-column representation — but a `geoarrow.wkb`-labeled column carrying EWKB is a known spec deviation: geoarrow-c and geopandas accept EWKB while GDAL rejects it, so treat ANY-SRID EWKB as a documented deviation (readers may reject it or silently drop the embedded SRID) rather than assuming GeoArrow reader support. - **Context:** PR #699 reviewers flagged that `TryCreate("GEOGRAPHY(3857)")` initially succeeded; the author added rejection + tests so GEOGRAPHY accepts only SRID 4326 or ANY. **Rule:** Databricks GEOGRAPHY supports only type-level SRID 4326 (WGS84) or ANY (`-1`) — any other fixed SRID (0, 3857, etc.) is invalid and must be rejected as malformed, whereas GEOMETRY accepts any nonnegative SRID or ANY; enforce and test this asymmetry at both the SQL-type-parse and the Arrow-tag-resolve boundaries. + +### 2026-10-21: learnings since 2026-10-20T17:30:25Z +- **Context:** PR #716 added dictionary-array support only to the parameter-binding path (a reviewer flagged that `extractGoValue`/`arrowTypeToDatabricksType` on the ingest path still fell through to `unsupported Arrow type`/`STRING`), and PR #718 then had to resolve dictionary encoding (`arrowValueType`) and add the `arrow.DICTIONARY` case across all four Go code paths to make ingest and bind consistent. + **Rule:** The Go driver has TWO parallel Arrow type-dispatch switches — the parameter-binding path (`parameter_binding.go`: `arrowValueToParameter`, `bindQuery`, `validateParameterType`) and the ingest path (`bulk_ingest.go`: `extractGoValue`, `arrowTypeToDatabricksType`, `buildInsertSQL`) — so any new Arrow type support (dictionary encoding, binary variants, etc.) must be applied to BOTH or you get a silent "works for ExecuteQuery/ExecuteUpdate, fails for ingestion" asymmetry; for dictionary arrays specifically, unwrap to the value type via a shared `arrowValueType` helper and resolve values with `dictionary.Dictionary()` + `GetValueIndex(idx)`. +- **Context:** PR #716 changed all Arrow binary variants (`BINARY`, `LARGE_BINARY`, `BINARY_VIEW`, `FIXED_SIZE_BINARY`) in `extractGoValue` to `hex.EncodeToString(...)` and made `buildInsertSQL`/`bindQuery` wrap every binary placeholder in `UNHEX(?)`, replacing an ad-hoc `fmt.Sprintf("%x")` that previously only handled `FIXED_SIZE_BINARY`. + **Rule:** Databricks parameters are sent as text, so arbitrary bytes must be hex-encoded (`hex.EncodeToString`) and the SQL placeholder wrapped in `UNHEX(?)` to round-trip losslessly — this applies to ALL Arrow binary types, not just fixed-size binary; a plain `?` placeholder or a raw `[]byte` value corrupts binary data. +- **Context:** PR #716's ingest/bind validation fixtures (`timestamp_ns`, `timestamptz_ns`) were marked `partial-support = true` with caveat "Nanosecond timestamps are truncated to microseconds," and `decimal_scale_negative` was hidden because scale must be 0..precision. + **Rule:** Databricks `TIMESTAMP`/`TIMESTAMP_NTZ` support only microsecond resolution, so Arrow nanosecond timestamps are silently truncated to microseconds on bind/ingest (declare these fixtures `partial-support` with a caveat rather than expecting exact round-trip); and `DECIMAL` scale must be between zero and precision (negative scale is unsupported). +- **Context:** PR #716 added `go/error_inspector.go` (`DatabricksErrorInspector`) wired via `DriverImplBase.ErrorHelper.ErrorInspector`, iterating `errors.As`/`errors.Unwrap` over `dbsqlerr.DBExecutionError` to copy SQLSTATE and map specific codes (e.g. 42P07→AlreadyExists) to ADBC statuses, and switched call sites from `ErrorHelper.Errorf` to `WrapInternal`/`WrapInvalidState`. + **Rule:** To surface Databricks SQLSTATE and map it to ADBC status codes in the Go driver, implement an `ErrorInspector` and set it on `ErrorHelper.ErrorInspector`, then use the `Wrap*` helpers instead of `Errorf` at call sites; when extracting SQLSTATE, loop through the wrapped error chain because a `DBExecutionError` wrapper can report an empty `SqlState()` even when its unwrapped cause carries one. +- **Context:** PR #709 regenerated `.github/workflows/*` via `adbc-gen-workflow` (driven by `generate.toml`), and reviewers flagged that regeneration silently dropped the explicitly-shared `go_test`/`go_release` concurrency group (shared-warehouse contention risk) and replaced SHA-pinned tooling (`go-licenses`, `dbc`, `adbc-drivers/dev`, `pre-commit`) with moving refs (`@master`, bare `git clone`, unpinned pip) — all in files headed `DO NOT EDIT`. + **Rule:** The Go CI workflows are auto-generated by `adbc-drivers/dev`'s `adbc-gen-workflow` from `generate.toml` and carry `DO NOT EDIT` — fix issues like unpinned supply-chain refs or lost pins in the upstream `dev` templates, NOT in this repo; and after any regeneration, diff for silently-dropped repo customizations (shared concurrency groups, SHA pins, version pins) since the generator can revert them without notice. +- **Context:** PR #718 reviewers noted the new dictionary code paths had no regression gate, pointing out that `go/parameter_binding_test.go` already builds Arrow arrays in memory (covering typed nulls, timestamps, binary, multi-batch binding) with no live warehouse required. + **Rule:** Go parameter-binding and Arrow type-dispatch branches can and should be unit-tested warehouse-free using in-memory Arrow array builders in `parameter_binding_test.go` (and the sibling ingest test); a new type branch validated only by live/manual checks has no CI regression gate, so add an in-memory case (ideally covering a non-null value, a null index, and a null dictionary value). From 2a61c9c8637210a07af7a1290a97683cb517f578 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 22 Oct 2026 17:31:40 +0000 Subject: [PATCH 14/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.claude/knowledge/learning.md b/.claude/knowledge/learning.md index da174548..73aab276 100644 --- a/.claude/knowledge/learning.md +++ b/.claude/knowledge/learning.md @@ -212,3 +212,7 @@ The E2E suite runs on whichever protocol the run configures (Thrift by default i **Rule:** The Go CI workflows are auto-generated by `adbc-drivers/dev`'s `adbc-gen-workflow` from `generate.toml` and carry `DO NOT EDIT` — fix issues like unpinned supply-chain refs or lost pins in the upstream `dev` templates, NOT in this repo; and after any regeneration, diff for silently-dropped repo customizations (shared concurrency groups, SHA pins, version pins) since the generator can revert them without notice. - **Context:** PR #718 reviewers noted the new dictionary code paths had no regression gate, pointing out that `go/parameter_binding_test.go` already builds Arrow arrays in memory (covering typed nulls, timestamps, binary, multi-batch binding) with no live warehouse required. **Rule:** Go parameter-binding and Arrow type-dispatch branches can and should be unit-tested warehouse-free using in-memory Arrow array builders in `parameter_binding_test.go` (and the sibling ingest test); a new type branch validated only by live/manual checks has no CI regression gate, so add an in-memory case (ideally covering a non-null value, a null index, and a null dictionary value). + +### 2026-10-22: learnings since 2026-10-21T17:31:03Z +- **Context:** PR #720 (a `chore: update dependencies` PR) carried a smuggled behavioral fix in `go/pkg/driver.go` log-level parsing: the switch had `case "warn":` with an empty body followed by `case "warning": logLevel = slog.LevelWarn`, so `ADBC_DRIVER_DATABRICKS_LOG_LEVEL=warn` silently kept the default level — only `warning` worked. + **Rule:** Go switch cases do NOT implicitly fall through, so an empty `case` body is a silent no-op rather than sharing the next case's body — collapse aliases into a single comma-separated label (`case "warn", "warning":`) and add a table test over the string→level mapping to guard the regression; also don't bury observable behavior changes in a `chore`/dependency-bump PR.