From e26dffc47cd5e65b3199480ef368a75dfde33a08 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 14 Aug 2026 17:42:27 +0000 Subject: [PATCH 1/4] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index 988c85f46..3169a1d82 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -10,3 +10,10 @@ configured in `.bot/config.yaml`: No learnings have been recorded yet. Dated sections are appended below by the retrospective flow. +## Entries + +### 2026-08-14: learnings since 2026-08-13T17:41:38Z +- **Context:** Author run #31744986912 fixed a parameter-bind bug where `DatabricksTypeUtil.getDatabricksTypeFromSQLType` collapsed both `Types.FLOAT` and `Types.REAL` to the 4-byte Databricks `FLOAT` wire type, silently narrowing FLOAT binds. + **Rule:** Per the JDBC spec (Appendix B type table), `Types.FLOAT` is a synonym for `DOUBLE` (8-byte double precision, Java `double`) and must map to the 8-byte wire type; only `Types.REAL` is 4-byte single precision (Java `float`) — never collapse FLOAT and REAL to the same Databricks type, and treat the declared SQL target type (not the bound value's native type) as what drives the wire type. +- **Context:** In author run #31744986912 the read-only diagnosis/plan phase spent a turn calling `edit_file` to create a test file (turn 44), only to discover editing tools are unavailable in that phase (turn 46) — the deliverable is the structured plan, not code. + **Rule:** The diagnosis/plan phase is read-only; don't attempt `edit_file`/write operations there. Defer all file creation and edits to the author_tests/fix phases and keep the diagnosis phase to reads, greps, and the structured plan output. From 545e97ff5ebf3cf337679122e39809769a3b2aef Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Tue, 18 Aug 2026 17:35:52 +0000 Subject: [PATCH 2/4] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index 3169a1d82..101d6921e 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -17,3 +17,9 @@ retrospective flow. **Rule:** Per the JDBC spec (Appendix B type table), `Types.FLOAT` is a synonym for `DOUBLE` (8-byte double precision, Java `double`) and must map to the 8-byte wire type; only `Types.REAL` is 4-byte single precision (Java `float`) — never collapse FLOAT and REAL to the same Databricks type, and treat the declared SQL target type (not the bound value's native type) as what drives the wire type. - **Context:** In author run #31744986912 the read-only diagnosis/plan phase spent a turn calling `edit_file` to create a test file (turn 44), only to discover editing tools are unavailable in that phase (turn 46) — the deliverable is the structured plan, not code. **Rule:** The diagnosis/plan phase is read-only; don't attempt `edit_file`/write operations there. Defer all file creation and edits to the author_tests/fix phases and keep the diagnosis phase to reads, greps, and the structured plan output. + +### 2026-08-18: learnings since 2026-08-17T17:33:39Z +- **Context:** PR #1582 wired `merge_group` triggers into required-check workflows (checkNextChangelog, releaseFreeze, prCheck) for a GitHub merge queue. + **Rule:** For a required status check to stay valid under a GitHub merge queue, register the workflow on the `merge_group` event; PR-semantic jobs that read `github.event.pull_request.*` should self-skip in the queue with `if: github.event_name != 'merge_group'` — a skipped required check counts as a pass under branch protection, so the queue is not blocked while the gate stays enforced at PR time. +- **Context:** In PR #1582 the engineer-bot re-posted the same "NEEDS HUMAN DECISION / blocked" verdict on successive replies of one review thread because the reviewer's finding required editing `.github/workflows/*.yml`, a non-writable path in this environment. + **Rule:** When a review finding targets a path the environment cannot write (e.g. `.github/workflows/*`), state once that it is agreed-but-not-actionable-here and stop; do not re-analyze or re-post a blocked verdict on each subsequent thread reply. From 03f291b655b6084c362881cb4ffefbf47fb13620 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 20 Aug 2026 17:35:56 +0000 Subject: [PATCH 3/4] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index 101d6921e..c742f7fd4 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -23,3 +23,7 @@ retrospective flow. **Rule:** For a required status check to stay valid under a GitHub merge queue, register the workflow on the `merge_group` event; PR-semantic jobs that read `github.event.pull_request.*` should self-skip in the queue with `if: github.event_name != 'merge_group'` — a skipped required check counts as a pass under branch protection, so the queue is not blocked while the gate stays enforced at PR time. - **Context:** In PR #1582 the engineer-bot re-posted the same "NEEDS HUMAN DECISION / blocked" verdict on successive replies of one review thread because the reviewer's finding required editing `.github/workflows/*.yml`, a non-writable path in this environment. **Rule:** When a review finding targets a path the environment cannot write (e.g. `.github/workflows/*`), state once that it is agreed-but-not-actionable-here and stop; do not re-analyze or re-post a blocked verdict on each subsequent thread reply. + +### 2026-08-20: learnings since 2026-08-19T17:33:03Z +- **Context:** PR #1629 added telemetry error-classification guardrails to CLAUDE.md and the PR template, covering any error emitted by the JDBC driver. + **Rule:** When adding or changing a driver-emitted error, use `DatabricksDriverErrorCode` (reuse a matching code or add a uniquely-numbered enum value), add a test asserting the emitted error name and numeric code, and record its driver/server/user classification in the maintainers' telemetry taxonomy — never infer the classification from the error name alone. From 861888a6588e9b92b2c18ed8e773a3a71d0bf36c Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Sat, 22 Aug 2026 17:32:47 +0000 Subject: [PATCH 4/4] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index c742f7fd4..c66ea7aab 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -27,3 +27,9 @@ retrospective flow. ### 2026-08-20: learnings since 2026-08-19T17:33:03Z - **Context:** PR #1629 added telemetry error-classification guardrails to CLAUDE.md and the PR template, covering any error emitted by the JDBC driver. **Rule:** When adding or changing a driver-emitted error, use `DatabricksDriverErrorCode` (reuse a matching code or add a uniquely-numbered enum value), add a test asserting the emitted error name and numeric code, and record its driver/server/user classification in the maintainers' telemetry taxonomy — never infer the classification from the error name alone. + +### 2026-08-22: learnings since 2026-08-21T17:33:33Z +- **Context:** PR #1652 fixed connections failing when a parameter appeared in both the JDBC URL and the `Properties` object; the root cause was Guava's `ImmutableMap.Builder.build()`, which throws on duplicate keys. The fix inserts properties first, then URL params, and calls `buildKeepingLast()` so URL wins. + **Rule:** When merging config from multiple sources into a Guava `ImmutableMap.Builder`, `build()` throws on duplicate keys — use `buildKeepingLast()` and insert entries in ascending precedence order (lowest-priority source first) so the highest-priority source wins. +- **Context:** PR #1621's new `BatchParameterSet` normalizes JDBC's 1-based parameter indexes to **zero-based** wire ordinals (`cardinal = index - 1`), while the existing SEA/Thrift path (`DatabricksPreparedStatement.setObject` → `mapToParameterListItem.setOrdinal`) forwards the raw **1-based** index. A reviewer flagged the off-by-one; the author confirmed the native-batch backend contract expects zero-based ordinals (first param = 0), whereas the current non-batch backend ignores the ordinal and relies on positional order. + **Rule:** Parameter ordinals diverge by execution path — native batching expects 0-based ordinals while the existing SEA/Thrift path sends 1-based JDBC indexes (tolerated only because that backend uses positional order); when wiring native batching through shared `setOrdinal` plumbing, verify the ordinal base against the backend contract and cover both paths with request-capture tests to avoid an off-by-one.