[peco-engineer-bot]

Summary

Automated fix for #505 — TIME (time64[ns]) type unhandled.

Added *arrow.Time32Type/*arrow.Time64Type arms to makeColumnValueContainer plus a new timeValueContainer (and timeUnitToNanos helper) that renders units-since-midnight as a UTC wall-clock time.Time; this resolves the "unhandled type time64[ns]" failure for SQL TIME columns. Unit tests pass and the full make test suite + make lint are green. The e2e test (TestE2ETimeTypeScan) was confirmed red against the live warehouse in the prior phase; it only skips here because this runner's bash subprocess scrubs credential-shaped env vars so the warehouse creds aren't reachable from the tool.

Root cause & plan

Root cause: On the Thrift Arrow path, a TIME column arrives in the Arrow IPC schema as time64[ns] (microsecond-precision TIME is still carried as time64 nanoseconds). arrowValueContainerMaker.makeColumnValueContainer (internal/rows/arrowbased/arrowRows.go, ~line 591+) switches on the concrete arrow.DataType and has arms for Date32/Timestamp/Decimal/String/List/Map/Struct/Null etc., but NO arm for *arrow.Time32Type or *arrow.Time64Type. The value hits the default arm and returns errArrowRowsUnhandledArrowType(t.String()) -> "databricks: arrow row scanner unhandled type time64[ns]". Because containers are built eagerly in makeColumnValuesContainers when the row scanner is created, this fails the whole result set before any row is returned. DATE and TIMESTAMP already have arms (dateValueContainer / timestampValueContainer); TIME has none.
Files: internal/rows/arrowbased/arrowRows.go, internal/rows/arrowbased/columnValues.go, driver_e2e_test.go
Planned coverage:

• Thrift-path e2e against the live warehouse: SELECT TIME '09:15:00.123456' must run and rows.Scan must succeed (no 'unhandled type time64[ns]' error), yielding a time-of-day time.Time with Hour()=9, Minute()=15, Second()=0, Nanosecond()=123456000 and UTC location (no date, no location). This is the exact reproducer from the issue and exercises the real server's time64[ns] encoding end-to-end. (time64 (primary, live e2e))
• Unit test: build an arrow time64 array (Nanosecond and Microsecond units) and run it through makeColumnValueContainer + the new container; assert the returned time.Time is the correct wall-clock time-of-day (e.g. nanoseconds-since-midnight converted per unit), anchored on a fixed reference date in UTC. Guards the unit->ns math and the 'no date/no location' rendering. (time64 container unit conversion (unit/offline edge))
• Unit test: build an arrow time32 array (Second and Millisecond units) through the new *arrow.Time32Type arm and assert correct time-of-day. Defensive: covers the time32 arm the fix adds alongside time64 so a time32 TIME would also scan rather than hit the default error. (time32 container (defensive edge))

Files changed

• internal/rows/arrowbased/time_test.go
• driver_e2e_test.go
• internal/rows/arrowbased/columnValues.go
• internal/rows/arrowbased/arrowRows.go

Test plan

• TestTime64ValueContainer (internal/rows/arrowbased/time_test.go) — fails (red) against the original code, passes (green) after the fix
• TestTime32ValueContainer (internal/rows/arrowbased/time_test.go) — fails (red) against the original code, passes (green) after the fix
• TestE2ETimeTypeScan (driver_e2e_test.go) — fails (red) against the original code, passes (green) after the fix

🤖 Generated by engineer-bot (bug-fix flow) — review before merge.

[peco-review-bot]

Verdict: 1 Medium

Solid, well-targeted fix for the unhandled time64[ns] TIME type — the new Time32/Time64 arms, container, and unit tests are coherent and release arrays correctly. One medium concern: .In(tvc.location) shifts a zoneless SQL TIME when a non-UTC WithLocation is configured, contradicting the container's stated intent and untested by the UTC-only unit tests.

[peco-review-bot]

Verdict: 1 Medium

Looks good — the TIME fix is correct and well-tested (unit→ns math is sound, the zoneless-UTC rendering is deliberate and matches the sibling date/timestamp containers, and the Time32/Time64 unit cases are covered). One medium: the kernel/SEA backend (internal/arrowscan) still errors on TIME, so the two backends now diverge for the same query despite the repo's cross-backend parity contract. Note also that a scanned TIME carries a spurious 1970-01-01 date (documented in the container comment, and the only reasonable anchor), which callers using .Format/.Date() will observe.

[peco-review-bot]

Verdict: 1 Low

Looks good — the time32/time64 TIME handling is correct, overflow-safe, lifecycle-consistent with the existing containers, and the Thrift and kernel paths render identically (parity test extended). One low note: a zoneless TIME is epoch-anchored on 1970-01-01, which would surface a spurious date if a TIME is ever rendered nested inside a complex type via marshal() — likely unreachable and consistent with existing nested-TIMESTAMP behavior.

[peco-review-bot]

Verdict: 1 Medium

One cross-backend concern: the fix special-cases nested TIME rendering on the Thrift side (marshalScalar → time-of-day string) but not on the kernel side (internal/arrowscan nested path still emits the full time.Time.String() with the 1970-01-01 epoch date), so a TIME inside a STRUCT/LIST/MAP diverges between the two backends — and no parity test covers the nested case. Top-level handling and the unit tests look correct.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

Verdict: 1 Medium · 1 Nit

Looks good overall. The Thrift-path fix for time64/time32 is correct and has thorough unit, parity and e2e coverage. One medium gap: the kernel column-type metadata was not updated alongside the new scan arms. Nit: timeUnitToNanos (arrowbased) and timeOfDayToTime (arrowscan) duplicate the unit→ns switch, and the PR description's file list leaves out the arrowscan changes.

Other findings

• ⚪ Nit — ValueString swallows the error from Value and returns "". marshalScalar then emits it as "", so a nested TIME inside a STRUCT/LIST/MAP that hits the unhandled-array path renders silently as an empty string instead of surfacing an error. Other nested paths, such as marshal/Value, propagate errors.

Suggested fix: have marshalScalar call Value itself and return the error, or have ValueString return (string, error). The error branch is unreachable in practice today because SetValueArray already validates the type, but returning the error keeps a future regression from turning into silently wrong data.

[peco-engineer-bot]

Add parity cases for TIME map keys.

Pushed fe0a9e5.

[peco-review-bot]

Verdict: 1 Medium · 1 Nit

The core fix looks correct. The new Time32/Time64 container arms fix the unhandled type time64[ns] error and keep the zoneless value in UTC. One medium concern: TIME column metadata now differs between the Thrift and kernel paths, and the parity test that should catch this doesn't cover TIME. Separately, the PR description lists only 4 changed files, but the diff also changes internal/arrowscan/* (kernel scan, nested JSON and map-key rendering, plus coltype) and arrowscan_parity_test.go. Reviewers should look at those kernel-path changes too.

Other findings

• ⚪ Nit — The e2e test checks that the scanned value parses as a time-of-day, but it doesn't check rows.ColumnTypes() (scan type / database type name) for the TIME column. It also has no nested case (e.g. SELECT named_struct('t', TIME '09:15:00')). Yet this PR adds a lot of nested-TIME rendering (marshalScalar, plus writeJSON and map keys in arrowscan) based on the assumed server format "09:15:00". One extra live assertion on a nested TIME and on the column metadata would confirm those assumptions against the real server encoding instead of only against synthetic arrays.

[peco-review-bot]

🟡 Medium — This makes the kernel path report TIME columns as DatabaseTypeName: "TIME" with ScanType time.Time. The Thrift mapping can't do the same: cli_service has no TTypeId_TIME_TYPE, so over Thrift the server declares a TIME column under some other id. getScanType / GetDBTypeName then give a different name, and probably a different scan type (string or unknown), while timeValueContainer.Value now returns a time.Time.

This causes two problems:

1. On the Thrift path, ColumnTypeScanType for a TIME column probably disagrees with the value that Next actually returns.
2. The two backends no longer agree on column metadata. This is exactly the drift TestColumnTypeInfoMatchesThriftMapping exists to catch, but the PR adds no time32/time64 case to that test, so the mismatch passes CI.

Suggested fix: check live which TTypeId the server reports for SELECT TIME '…'. Then either align getScanType for that id with ColumnTypeInfoFor, or add a parity case that writes down the intended difference.

[peco-engineer-bot]

⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.

Blocked: the fix depends on which TTypeId the server declares over Thrift for SELECT TIME '…', and only a live warehouse can show that. This job has no warehouse credentials. cli_service has no TIME id, and the Thrift value comes from the Arrow type: time64 goes to timeValueContainer, which returns a time.Time. getScanType/GetDBTypeName go by the TTypeId, so on Thrift they almost certainly disagree with that value. TestColumnTypeInfoMatchesThriftMapping has no time32/time64 case, and TestE2ETimeTypeScan only checks the value, not ColumnTypes(). I didn't add a parity row because the id would be a guess. A human needs to: (1) run SELECT TIME '09:15:00.123456' over Thrift and record the column's PrimitiveEntry.Type, ColumnTypeScanType and ColumnTypeDatabaseTypeName; (2) decide whether to align getScanType/GetDBTypeName with ColumnTypeInfoFor's TIME/time.Time (e.g. by looking at the Arrow type) or keep the difference on purpose; (3) add a {"time64", Time64us, } row (or a documented exception) to TestColumnTypeInfoMatchesThriftMapping, and optionally assert ColumnTypes() in TestE2ETimeTypeScan.