[Mandukhai-Alimaa]

Fix Go ingest error codes and binary ingestion. Correct validation fixtures and preserve timestamp precision when splitting SQL.

Temporarily skip two ingestion tests that time out, the catalog-wide GetObjects test that repeatedly fails after long CI scans, and timestamp precisions 7–9 because the validation warehouses have nanosecond timestamp types disabled.

Validated: build, targeted live validation, skip checks, and pre-commit.

Stacked on #713. Dictionary binding and ingestion are handled in #718; their validation cases require that PR.

[peco-review-bot]

Verdict: 1 Low

Looks good — the ingest error-code mapping, binary hex/UNHEX handling, and dictionary parameter binding are all correct. Verified that driverbase.ErrorHelper is a value type so the errorHelper := s.ErrorHelper copy-and-override in executeIngest does not mutate shared state, that dictionary nulls are handled by the IsNull check preceding the new DICTIONARY case, and that the error_inspector.go unwrap loop terminates. The four newly-skipped tests (large-batch/many-column ingest, catalog-wide GetObjects, ns-precision timestamps) are each documented as temporary with concrete CI justifications, which is reasonable. One low-severity asymmetry noted inline: the ingest path still lacks the dictionary support now present in parameter binding.

[peco-review-bot]

🔵 Low — This PR adds dictionary-encoded array support to the parameter-binding path (parameterValueType + the new arrow.DICTIONARY case in arrowValueToParameter), but the ingest path is left asymmetric. extractGoValue has no DICTIONARY case, so it falls through to the default branch and returns unsupported Arrow type; arrowTypeToDatabricksType similarly falls back to STRING for a dictionary column. As a result, binding the same dictionary-encoded Arrow data succeeds for ExecuteQuery/ExecuteUpdate but fails for ingestion. If dictionary ingestion is intentionally out of scope for this PR, consider resolving the value type here too (mirroring parameterValueType) in a follow-up, or at least documenting the gap.

(Anchored to the nearest changed line — see the description for the exact location.)

[peco-review-bot]

Verdict: 1 Low

Looks good — the error-helper refactor, binary hex-ingestion, and InspectError unwrap loop are all correct, and ErrorHelper being a value type means the per-ingest inspector copy doesn't mutate shared state. One low-severity note: the ingest-mode 42703 → ALREADY_EXISTS remapping conflates a schema-conflict with a legitimately-missing/misspelled column. The rest of the diff is test fixtures and documented temporary skips.

[peco-review-bot]

🔵 Low — In ingest mode, UNRESOLVED_COLUMN (SQLSTATE 42703) is remapped from StatusInvalidArgument to StatusAlreadyExists. This conflates two genuinely different ingest failures: (a) the target table pre-exists with a schema that conflicts with the bound input, and (b) the caller simply named a column that doesn't exist (e.g. a typo in a bound field name, or an append into a table that legitimately lacks that column). Case (b) is a true invalid-argument condition, but it will now surface to the ADBC caller as ALREADY_EXISTS, which is misleading and may hide a real input mistake. The comment documents the intended case (a), and this is clearly deliberate to satisfy the validation fixture, so flagging as low — but consider narrowing the override (e.g. only for Create/CreateAppend modes where a pre-existing table is the plausible cause) rather than applying it to every ingest path.

[lidavidm]

Can we file a report upstream?

[Mandukhai-Alimaa]

tobymao/sqlglot#8718

[lidavidm]

File an issue for this? Can we leverage some of the logic we already have for this case?

[Mandukhai-Alimaa]

#193. There is already issue for it, I will work on it the after the CVE fix release.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

Verdict: 1 Low

Looks good — the ingest error-code and binary-ingestion fixes are correct, with the ingest-specific ErrorInspector properly scoped to a value-copy of ErrorHelper, and the token-based split_statement and new/edited fixtures all check out. One low note on the 42703 → StatusAlreadyExists ingest remapping, which is intentional but semantically surprising and worth confirming against the validation suite's expectation.

[peco-review-bot]

🔵 Low — During ingest, a 42703 (UNRESOLVED_COLUMN) server error is remapped from StatusInvalidArgument to StatusAlreadyExists. This is a surprising conflation: 42703 fires whenever a referenced column can't be resolved against the destination table, and the driver builds the INSERT column list from the input Arrow schema — so this maps "input has a column the table doesn't" to a status that callers normally read as "the table already exists." That is a reasonable interpretation for the append-into-incompatible-table case the comment describes, but it will also mask genuinely different column-resolution failures under the same status. If the validation suite specifically expects ALREADY_EXISTS here this is intended — worth a confirming note, since StatusAlreadyExists is a lossy label for "schema conflict."

[peco-review-bot]

Verdict: 1 Low

Looks good — the error-helper refactor to Wrap* + DatabricksErrorInspector correctly preserves SQLSTATE and remaps status, the hex/UNHEX binary ingestion is consistent across SQL generation and value extraction (nulls short-circuit before encoding, empty bytes round-trip correctly), and the executeIngest ingest:true inspector is a value-copy that doesn't leak into the shared helper. One low-severity note on the breadth of the 42703→AlreadyExists ingest mapping. Nit: the new token-based split_statement silently returns [] for comment-only/whitespace-only input and will raise if sqlglot.tokenize hits malformed SQL — a minor behavior change from quirks.split_statement, acceptable for controlled fixtures.

[peco-review-bot]

🔵 Low — During ingest, SQLSTATE 42703 (UNRESOLVED_COLUMN) is remapped to StatusAlreadyExists. The generated INSERT is driver-built from the bound schema, so in the common case a missing column really does mean a schema conflict with an existing table — but 42703 can also surface for unrelated reasons (e.g. an identifier the driver quotes/qualifies unexpectedly, or a server-side resolution quirk), and those would now be reported to callers as AlreadyExists rather than a more accurate InvalidArgument/Internal. Worth confirming the mapping is narrow enough in practice, or consider also matching on the message text before overriding to AlreadyExists.