[peco-engineer-bot]

Fixes #506: preserve native Arrow TIMESTAMP_NTZ wall-clock fields and report TIMESTAMP_NTZ on both backends. Returned NTZ time.Time values use UTC as their location; their represented instant and .UTC() result are unchanged. Zoned TIMESTAMP values continue to use the session location.

Thrift type names now come from cached server Arrow metadata, so they remain stable before iteration and after EOF. The kernel also honors logical NTZ metadata when its physical Arrow timestamp carries UTC. The docs describe the legacy string, non-Arrow, and missing-server-schema limitations.

Validation: full Go suite, tagged kernel unit tests, and focused live Thrift/kernel timestamp tests on pecotesting passed. Standard lint reports zero issues; kernel-tagged lint reports no new issues. Regression tests include captured server schemas, metadata lifecycle, and fallback handling.

[peco-review-bot]

Verdict: 1 Medium · 2 Low

Core logic is sound — the TIMESTAMP_NTZ distinction (type-name override from the Arrow schema's TimeZone and suppression of the .In(location) shift) is correct and index-aligned on both the Thrift and kernel paths, with good test coverage. Main issue is a stale README that still documents the old shifting behavior (doc.go was updated, README was not); plus a leftover scratch test file and an uncovered non-default legacy string-timestamp path.

Other findings

• 🟡 Medium — Stale documentation: this PR changes TIMESTAMP_NTZ to keep its wall clock un-shifted, and updates doc.go accordingly ("TIMESTAMP_NTZ (a zone-less wall clock kept as-is)"), but the README still describes the old, now-incorrect behavior — it lumps TIMESTAMP / TIMESTAMP_NTZ (shifted into the session time zone) together. After this change that statement is wrong for NTZ and directly contradicts doc.go. Please update the README Data types section to match the new contract (TIMESTAMP is an instant shifted into the session time zone; TIMESTAMP_NTZ is a zone-less wall clock kept as-is).

[peco-review-bot]

Verdict: 1 High · 1 Medium · 2 Low

The fix itself is sound, but the PR leaves some things inconsistent with it: a kernel e2e test that will fail, two stale docs, and gaps in test coverage. The NTZ split based on the Arrow TimeZone is applied consistently on the Thrift and kernel paths. However, TestKernelE2ETimestampNTZ still expects the old shift, and the user-visible change to NTZ values and their Location() is not called out anywhere.

[peco-review-bot]

Verdict: 2 Medium · 1 Low

The value fix and the kernel-side type-name fix look right. Two medium concerns remain on the Thrift path: the NTZ type name depends on whether a page is currently loaded, and the assumption about what TimeZone the server puts in its Arrow schema has not been checked against a live warehouse.

[peco-review-bot]

Verdict: 1 Medium

The value-rendering fix for TIMESTAMP_NTZ looks correct and consistent on both backends, with one medium issue. On the Thrift path the TIMESTAMP_NTZ type name is read from the current row scanner, so it falls back to TIMESTAMP before the first fetch when there are no direct results, and again after the result set is exhausted.

[peco-review-bot]

Verdict: 2 Low

Looks good overall, with two low-severity concerns. Both backends now split NTZ from zoned TIMESTAMP consistently at the top level, and the tests cover it well, including captured schemas, the metadata lifecycle and fallbacks. The concerns are about nested NTZ on the kernel and about the Thrift type-name override not matching how values are decoded on the non-Arrow path.

[peco-review-bot]

Verdict: 2 Low

Looks good overall, with two low-severity notes. Value handling and type-name reporting for NTZ match between Thrift and the kernel backend: both read the TimeZone on the server's Arrow schema, and the fallback schema built from TTableSchema uses Timestamp_us (UTC zone), so zoned TIMESTAMPs are still shifted into the session location. Nits: the ScanCell and renderJSONString doc comments still say loc applies to all TIMESTAMP leaves, and the kernel test loads its fixture from another package's directory (../../arrowscan/testdata), which will break if that directory moves.

[peco-review-bot]

Verdict: 1 High

One high-severity problem on the Thrift fallback path. The kernel and arrowscan NTZ handling and the cached Thrift type-name overrides look correct and well-tested. But when the server sends no Arrow schema, the driver builds one itself with zone-less timestamps, so zoned TIMESTAMP columns also skip the session-location shift. That contradicts the new docs.

[peco-review-bot]

Verdict: 1 Low

Looks good overall, with one low concern. Both backends now handle NTZ the same way: Thrift checks for an empty Arrow TimeZone, and the kernel checks for NTZ metadata (top-level and nested), and both have good unit and e2e coverage. The low finding is that when ResultFormat is unset, the Thrift type-name cache can report TIMESTAMP_NTZ for columnar results whose values are still shifted into the session location, which doesn't match what the code comment promises.

[peco-review-bot]

✅ No issues identified by the review bot.

[vuanhphung]

@bot

Finding 1 (internal/arrowscan/coltype.go:187):
childLocation only fires when a child field carries databricks.type_name=TIMESTAMP_NTZ, but the kernel only adds that key to top-level fields (augment_schema_with_manifest in reader/mod.rs; json.rs:239-246 lists it as a known limitation). The kernel fixture shows NTZ arriving as timestamp[us, tz=UTC], so a nested NTZ child probably arrives tz=UTC with no metadata and is still shifted, while Thrift (empty tz, native complex types on by default) now isn't. Could you check named_struct('n', TIMESTAMP_NTZ …), array(TIMESTAMP_NTZ …) and map(TIMESTAMP_NTZ …, 1) live on both backends with a non-UTC session timezone? TestScanCellNestedKernelNTZKeepsWallClock builds child metadata the kernel never sends.

Finding 2 (internal/rows/rows.go:422):

The captured Thrift schema already carries Spark:DataType:SqlName=TIMESTAMP_NTZ (and the kernel copies the same key), yet the override here and wallClock in arrowRows.go:655 each infer NTZ from TimeZone == "" on their own. The same check also lives in coltype.go and arrowscan.go. Could we add one arrowscan helper that checks the explicit metadata first and falls back to tz? That keeps the type name and the value handling in sync, and stops the open "does the server always set tz" question from blocking.

[peco-engineer-bot]

Pushed 9f08fef to ai/issue-506 addressing your request.

Started from 332032f. F2 done: added arrowscan.IsTimestampNTZField. It checks databricks.type_name, then Spark:DataType:SqlName, and falls back to empty tz only with no metadata. It is now used by rows.go:422, coltype.go, ScanCell and childLocation. Thrift containers now take the whole field (via makeFieldValueContainer), so top-level and nested children see metadata. Added tests. make test and make lint pass; gofmt flags only generated cli_service files. F1 not fixed, and the live NTZ checks were not run (no warehouse or kernel build here). If kernel nested NTZ arrives as tz=UTC with no metadata, the driver can't tell it from TIMESTAMP, so it is still shifted. That needs a kernel change. The nested-kernel test models metadata the kernel doesn't send yet.

[peco-review-bot]

Verdict: 1 Low

Looks good overall — one low concern. On the kernel path, an Arrow timestamp with no time zone keeps its wall clock even when its field metadata labels it a zoned TIMESTAMP. That disagrees with the reported type name and with how Thrift handles the same field. The Thrift metadata caching, the nested child handling, and the docs/CHANGELOG updates look correct and are well tested.

[peco-review-bot]

Verdict: 2 Low

Looks good: the NTZ decision goes through a single check (IsTimestampNTZField), so the reported type name and the value handling agree across Thrift top-level/nested containers, the kernel rows, and the field-less arrowscan entry points. I found two low-severity test-coverage gaps and no correctness issues.

[peco-review-bot]

Verdict: 1 Low

Looks good. There is one low-severity concurrency concern in the lazily-built Thrift type-name override cache. The NTZ check is shared across type naming, Thrift containers, kernel top-level and nested scanning, and the fixtures captured from live server schemas give good coverage.

[peco-review-bot]

Verdict: 1 Low

Looks good. The NTZ check is shared through IsTimestampNTZField, so the type name and the value handling stay in sync on both backends. Nested children, map keys, the fallback when the server sends no Arrow schema, and the metadata lifecycle on Thrift are all handled and tested. The only issue is one low-severity case of comments no longer matching the code, which use metadata first. Nit: no test covers a TIMESTAMP_NTZ map key through writeMapJSON/scalarForJSON on the kernel scanner, though the code path looks correct.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity note. Thrift and kernel now use the same IsTimestampNTZField check for both the reported type name and the value handling. The Thrift type-name override is built from server metadata that's cached once, so it gives the same answer before iteration and after EOF. Nested children, map keys, and the fallback when there's no schema are all handled and tested. The only issue is a stale comment in one test.

[peco-review-bot]

✅ No issues identified by the review bot.