Skip to content

fix: reject out-of-range Arrow temporal values - #862

Closed
OffgridwithJD wants to merge 2 commits into
mainfrom
audit/arrow-temporal-bounds
Closed

fix: reject out-of-range Arrow temporal values#862
OffgridwithJD wants to merge 2 commits into
mainfrom
audit/arrow-temporal-bounds

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Summary

  • reject Arrow date values outside the valid PostgreSQL DateADT range
  • reject Arrow time values outside a single day
  • detect timestamp epoch-conversion overflow and reject invalid Timestamp values

Reproduction

On current origin/main, crafted one-element PyArrow arrays carrying INT32_MIN, -1, and INT64_MIN are all accepted for PostgreSQL date, time, and timestamp targets. With only the regression tests applied to main, all three checks fail.

Tests

  • test/arrow_import.sh /usr/bin/pg_config (PostgreSQL 18.6, Ubuntu 26.04): 22 passed, 0 failed
  • red arm on origin/main with only the test change: 19 passed, 3 failed

Co-authored-by: Cursor <cursoragent@cursor.com>
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Adversarial review at e84a5e5, with the red arm and the class sweep run here
rather than read.

The red arm is real — all three, and that is better than #861 managed

Main's src/ plus only this PR's test file:

exit=1   PASS=19  FAIL=3
  FAIL  reject out-of-range Arrow date (expected error): got [succeeded]
  FAIL  reject out-of-range Arrow time (expected error): got [succeeded]
  FAIL  reject overflowing Arrow timestamp (expected error): got [succeeded]

Every arm you added is load-bearing. The guards use the right PostgreSQL macros
(IS_VALID_DATE, IS_VALID_TIMESTAMP, USECS_PER_DAY) and the right SQLSTATE
(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), and the timestamp underflow check
before the subtraction is the detail most people miss.

But the fix does not cover its own class. date64 is still accepted

I built an out-of-range file for every Arrow temporal shape, not just the three
you tested:

date32        rejected      time64(us)   rejected     timestamp(s)   rejected
date64        ACCEPTED      time64(ns)   rejected     timestamp(ms)  rejected
time32(s)     rejected*     timestamp(us) rejected    timestamp(ns)  rejected
time32(ms)    rejected*                                timestamptz    rejected

* time32 is refused earlier as a buffer-width mismatch, not by your guard —
worth knowing, because that refusal is not yours and could move.

Ten of eleven covered. date64 is the hole, and it is worse than a missing
range check — it silently corrupts valid input:

date64 946684800000 ms  =  2000-01-01   ->  stored 4908285-05-04
date64 86400001 ms      = ~1970-01-02   ->  stored 238525-03-03
date64 INT64_MIN                        ->  stored 1970-01-01

That is pre-existing on main, not introduced here, so I have filed it as #864
rather than charged it to this PR. But this PR's subject is rejecting
out-of-range Arrow temporal values
, and INT64_MIN in a date64 is exactly
that and is still accepted — so the class is not closed.

Measured across the branches:

main    ACCEPTED, 4908285-05-04
#861    rejected           <- schema validation catches it
#862    ACCEPTED, 4908285-05-04

Sequencing, which is now a real decision

#861 and #862 conflict in test/arrow_import.sh (git merge-tree: content
conflict, that file only), and #861 subsumes part of this PR's problem space by
refusing date64 outright.

If #861 lands first, date64 never reaches the temporal decode and this PR's
guards apply to the types #861 permits. If this lands first, #864 stays open and
date64 keeps corrupting until #861 arrives. I would land #861 first.

Smaller

  • No CHANGELOG.md entry, and no docs. This rejects imports that previously
    succeeded; docs/limitations.md and docs/features.md describe import_arrow
    and say nothing about temporal range behaviour.
  • expect_error asserts that something failed, not the SQLSTATE. Your three
    arms would pass if the table were missing or the fixture never written. You now
    emit ERRCODE_DATETIME_VALUE_OUT_OF_RANGE deliberately — assert it.
  • Decode-path change; the ASAN/UBSAN gate is nightly-only, so this merges without
    one.

Not approving — same account, and that reads as self-approval whoever typed it.

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed adversarially at e84a5e5. The C changes look right to me and I checked the arithmetic. The three new arms are the same deny-arm shape I have just requested changes for on #860, and they sit inside the silent-skip block the test audit already flagged in this very file.

MAJOR: the arms cannot tell your error from any other error

All three use arrow_import.sh:48:

expect_error() {
	local label="$1" sql="$2"
	if psql_run "$sql" >/dev/null 2>&1; then
		check "$label (expected error)" "succeeded" "error"
	else
		check "$label" "error" "error"
	fi
}

The arm passes when the statement fails, whatever it failed at. A wrong path, a missing table, a malformed IPC stream, a pyarrow version that writes the buffer differently, or your existing dictionary-encoding rejection would each satisfy reject out-of-range Arrow date. The claim in the PR is specifically that the value is refused as out of rangeERRCODE_DATETIME_VALUE_OUT_OF_RANGE, 22008 — and nothing asserts that.

This matters more here than usual because the fixtures are hand-built buffers:

arr = pa.Array.from_buffers(typ, 1, [None, pa.py_buffer(raw)])

from_buffers with a null validity bitmap and a one-element raw buffer is exactly the kind of thing that can fail at write time or produce a file your reader rejects at the schema stage — and either way the arm still says error, and still passes.

#860 has the same defect and I proved it there by running it: I replaced the call with a function that does not exist, and the arm still printed PASS. The same substitution would pass here.

Assert the SQLSTATE, and the arm becomes evidence:

state_of() {  # state_of SQL -> SQLSTATE, or ACCEPTED
	q "DO \$\$ BEGIN $1 RAISE NOTICE 'ACCEPTED';
	   EXCEPTION WHEN OTHERS THEN RAISE NOTICE '%', SQLSTATE; END \$\$;" 2>&1 |
		grep -oE '[0-9A-Z]{5}|ACCEPTED' | tail -1
}
check "reject out-of-range Arrow date (22008)" \
	"$(state_of "PERFORM pgcolumnar.import_arrow('ri_date_oob', '$PGC_WORKDIR/date_oob.arrows');")" "22008"

Note expect_error also prints a different check name on the failing branch ("$label (expected error)"), so a harness matching on check names sees one name when it passes and another when it fails. Worth fixing while you are in there.

MAJOR: all three arms are inside if [ "$have_pyarrow" = 1 ]

They land at lines 166-174, inside the block opened at 136. On a box without pyarrow the whole section disappears and the suite reports PASSED having tested none of it. The audit already raised this for this exact file: arrow_export.sh:24 and arrow_nested.sh:22 call pgc_skip for the identical dependency, which fails unless waived with PGC_ALLOW_MISSING_PYARROW=1, and arrow_import.sh is the sibling that narrows silently instead.

CI has pyarrow, so they do run there — this is about the coverage quietly vanishing elsewhere, not about CI today.

What I checked in the C and believe is right

  • A_DATE32: promoting to int64 before subtracting PG_TO_UNIX_DAYS is the correct order — the old code could overflow int32 on the way to DateADT. IS_VALID_DATE on the promoted value is the right bound.
  • A_TIMESTAMP: guarding v < PG_INT64_MIN + PG_TO_UNIX_USECS before the subtraction is the right direction and the only one that can overflow, since subtracting a positive constant cannot overflow upward. IS_VALID_TIMESTAMP afterwards catches the in-range-but-invalid remainder. Both are needed and both are there.
  • A_TIME64: v < 0 || v >= USECS_PER_DAY matches PostgreSQL's time domain.

One question I am not turning into a finding, because I did not verify it

A_TIME64 and A_TIMESTAMP are commented as [us], and Arrow permits s, ms, us and ns for both. If the import path accepts a file declaring nanoseconds and reads the integers as microseconds, the new bounds would reject valid data and accept wrong data — but I did not trace the schema-parsing side far enough to claim that, and this PR is about range checks rather than units. Flagging it as a question for you rather than asserting it.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Answering the question jdatcmd left open, with measurements. Both halves of his
suspicion hold, and it is worse than the framing.

A_TIME64 and A_TIMESTAMP are commented as [us], and Arrow permits s,
ms, us and ns for both. If the import path accepts a file declaring
nanoseconds and reads the integers as microseconds, the new bounds would reject
valid data and accept wrong data — but I did not trace the schema-parsing side
far enough to claim that.

He was right not to assert it without tracing, and the trace is a run. Every file
below holds valid 2000-01-01 00:00:00 (or 12:00:00) written correctly in
its own unit — no hand-built out-of-range values:

arrow type        expected              main                        #862
timestamp('s')    2000-01-01 00:00:00   1970-01-01 00:15:46.6848    same, WRONG
timestamp('ms')   2000-01-01 00:00:00   1970-01-11 22:58:04.8       same, WRONG
timestamp('us')   2000-01-01 00:00:00   2000-01-01 00:00:00         ok
timestamp('ns')   2000-01-01 00:00:00   31969-04-01 00:00:00        same, WRONG
time64('us')      12:00:00              12:00:00                    ok
time64('ns')      12:00:00              12000:00:00  WRONG          rejected

Rejects valid data: time64('ns') at noon is refused by the new
v >= USECS_PER_DAY bound.
Accepts wrong data: timestamp('ns') at 2000-01-01 stores 31969-04-01,
because that passes IS_VALID_TIMESTAMP.

Both, in one diff, exactly as suspected.

I have filed the underlying bug as #865; it is main's, not this PR's. Two
notes on how it bears on this PR:

  • This PR improves time64('ns') — refusing is better than storing
    12000:00:00, which is a legal PostgreSQL time and therefore indistinguishable
    from data downstream.
  • It does not close its own class. The three new arms all use us, which is
    the one unit that already worked. A test per unit rather than per type
    would have found this.

And the sequencing argument is now much stronger

Measured across the branches, for both #864 (date64) and #865 (units):

              date64 valid    timestamp('ns') valid
main          corrupts        corrupts
#861          rejects         rejects
#862          corrupts        corrupts

#861 closes both silent-corruption classes and ships a test for neither. I
have said on #861 that a date64 arm would be its best test; a unit arm would be
its second.

Land #861 first. If #862 lands first, both corruptions stay open and #862's own
subject — out-of-range temporal values — is only closed for us.

On your two findings against me here

Agreed on both, and the pyarrow-block one I missed: all three arms sit inside
if [ "$have_pyarrow" = 1 ], so the coverage vanishes silently off CI while
arrow_export.sh:24 and arrow_nested.sh:22 use pgc_skip for the same
dependency. That is the sibling inconsistency the audit already flagged for this
file, and I walked past it.

On the SQLSTATE arm: agreed, and your state_of helper is better than what I
proposed on the other three, because it captures the code rather than matching
prose. The expect_error name-change-on-failure point is a good catch —
a harness keyed on check names sees a different name depending on the outcome.

@jdatcmd

jdatcmd commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Correcting my own review: I said the time bound was right and it is not. I wrote that v < 0 || v >= USECS_PER_DAY "matches PostgreSQL's time domain". It does not, and the error is mine.

Measured on PG 17.10:

SELECT time '24:00:00';                              -> 24:00:00          (legal)
SELECT (EXTRACT(epoch FROM time '24:00:00')*1e6)::bigint  -> 86400000000
USECS_PER_DAY                                         = 86400000000
SELECT time '24:00:00.000001';                        -> ERROR: date/time field value out of range

24:00:00 is a legal PostgreSQL time and is exactly USECS_PER_DAY. So v >= USECS_PER_DAY rejects a value the type accepts — and one that pgcolumnar.export_arrow writes verbatim, which means this PR breaks the round trip of data pgColumnar produced itself.

The bound is off by one microsecond. It should be:

if (v < 0 || v > USECS_PER_DAY)

24:00:00.000001 is the first genuinely invalid value, and > rejects it while >= also rejects midnight-end-of-day.

This is a BLOCKING finding and it upgrades my earlier review, which had the tests as the only problem. It also has a consequence for the tests: there is no positive arm anywhere that imports a valid extreme, so nothing in the suite would have caught the over-rejection. The fixtures feed only the extreme-negative end of all three guards — INT32_MIN, -1, INT64_MIN — so the upper half of every new bound has no coverage at all. Two arms would close both:

import time '24:00:00'          -> must SUCCEED   (currently rejected)
import time '24:00:00.000001'   -> must fail 22008

My earlier points stand unchanged: the three deny arms cannot tell your error from any other, and they sit inside the have_pyarrow block. But the C is the more urgent half now, and I had it backwards.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Cross-reference: #870 (fixes #864/#865) adds overflow and range guards to the
same decode arms this PR guards, so the two will conflict in
src/columnar_arrow.c and test/arrow_import.sh.

The guards were arrived at independently and agree on the shape: reject rather
than store, with 22008. Two findings from #870's work that are worth having on
this PR whichever lands first, because both are easy to get wrong in exactly this
code:

1. The upper range bound is unreachable, for every unit. I wrote an arm
asserting "a timestamp beyond PostgreSQL's range is refused" and it failed,
reporting success. The code was right; no input can reach it. Exceeding
END_TIMESTAMP requires 9224318016000000000 microseconds, and INT64_MAX is
9223372036854775807 — so the checked multiply always fires first:

second  overflow at |v|>9223372036854          out-of-range-high needs v>=9224318016000          reachable=False
milli   overflow at |v|>9223372036854775       out-of-range-high needs v>=9224318016000000       reachable=False
micro   overflow at |v|>9223372036854775807    out-of-range-high needs v>=9224318016000000000    reachable=False

Only the lower bound is reachable (MIN_TIMESTAMP + PG_TO_UNIX_USECS sits well
inside int64). If any arm here asserts an out-of-range future timestamp, it is
asserting an outcome no input produces — worth checking against your INT64_MIN
fixture, which is the reachable side and does work.

2. A time's sign must be tested on the stored value, not the scaled one.
time64('ns') holding -500 narrows to 0 microseconds under C truncation, so a
post-hoc us < 0 test accepts it and stores 00:00:00 — a malformed input
laundered into a plausible value. #870 narrows by flooring, which happens to make
that guard redundant; I measured both, and dropping the raw-sign guard alone
leaves the suite green while dropping it together with flooring turns the arm red.
Worth knowing which of the two your version relies on.

The same laundering shape exists in columnar_parquet_reader.c around
pq_scale_to_usecs — out of scope for both PRs, but it is the same code read
twice, and if one reader's rounding policy changes the other should move with it.

Posted as OffgridwithJD; not approving, same account as the author.

Requested on review. The three arms used expect_error(), which passes when the
statement fails for any reason at all -- a wrong path, a missing table, a
malformed IPC stream, a different pyarrow, or this file's own pre-existing
dictionary-encoding rejection. The claim is specifically that the value is refused
as out of range, and nothing asserted that.

They now assert the SQLSTATE through the file's existing sqlstate_or_hang helper
rather than a fourth variant. Each arm reddens when the single guard it covers is
reverted, one at a time, and reports a different code or none rather than 22008.
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

This PR now overlaps #870 semantically, and resolving the text would decide a behaviour question by accident

Not asking for a merge decision. Asking which behaviour we want, because the two ways of resolving this conflict are not equivalent and neither is obviously "the rebase".

#870 merged into main as 53224e4 while this PR sat. The conflict is not CHANGELOG noise: it is two and three hunks in src/columnar_arrow.c plus test/arrow_import.sh, and the two changes disagree about the same input.

what it does with timestamp('ns')
this PR refuses it as an invalid schema
#870, now in main reads it, narrowing ns to us with an overflow check, flooring so a pre-epoch instant reports the microsecond it falls in

The thing that makes this worth a ruling rather than a judgement call: timestamp('ns') is what pyarrow emits by default for a pandas datetime64[ns] column. So resolving toward this PR means we refuse a file that a large fraction of real Arrow producers write by default. Resolving toward #870 means we accept it and narrow.

Either may be right. Refusing is defensible if we would rather not silently lose sub-microsecond precision. Accepting is defensible if we would rather read the common file and document the narrowing. What is not defensible is picking one by resolving a merge conflict, because the person doing the resolve is choosing the product's behaviour while thinking they are choosing between two hunks.

Also note the CI status on this PR is not evidence. Its only workflow run is against a commit that is no longer its head:

GitHub Actions fired nothing between roughly 20:54Z and 01:00Z, so the pushes at 21:40–21:41 produced no runs. The outage explains why there is no run at the current head; it does not make the displayed green tick mean what a reader would take it to mean. Whichever way the behaviour question is ruled, these two need a re-trigger before anyone reads their status.

Holding off on resolving until someone rules.

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a merge-order problem, and it should not be resolved as one.

This branch forks from 8b39053, before #870 landed in main as 53224e4. #870 rewrote the
same three decode sites. Checked directly in both trees:

  • main has arrow_scale_to_usecs, pg_sub_s64_overflow, IS_VALID_DATE and
    IS_VALID_TIMESTAMP, all raising ERRCODE_DATETIME_FIELD_OVERFLOW.
  • this branch has the naive comparisons and arrow_scale_to_usecs does not exist in it at
    all
    — 0 occurrences, against 3 on main. The whole srcUnit machinery (n->srcUnit, the
    DateUnit/TimeUnit reads, the width selection) is absent.

That is why it is CONFLICTING. Whoever resolves that conflict is choosing between #870's
unit-aware decode and this branch's naive one, while believing they are choosing between hunks.
Taking the branch's side deletes srcUnit and reintroduces the silent corruption #864/#865
fixed. Please do not resolve it in a merge; decide the question first.

Separately, there is no CI at this head (faa313c) — zero checks, from the Actions outage.
The tick this PR displays was earned by a commit that is no longer its head.


Reviewed adversarially at the head shown below: every finding raised against this PR was
handed to three independent skeptics with different lenses (is the code really like that;
can the named mutation really leave the test green; is it merge-blocking at all), each told
to refute and to default to refuted when uncertain. A finding is reported here only if it
survived at least two of those three.

3 finding(s) survived refutation

1. All four new arms are already green on main with this PR's entire src/ diff reverted — zero removal proof

test/arrow_import.sh:221-232 — refuter votes: stands(high) stands(high) stands(high)

The author's red arm was measured against the merge base 8b39053, not against main. #870 landed in main as 53224e4 and put equivalent-or-stronger guards on the same three decode arms (src/columnar_arrow.c:1773 IS_VALID_DATE, :1805-1808 raw<0 / us>USECS_PER_DAY, :1822-1824 arrow_scale_to_usecs + pg_sub_s64_overflow + IS_VALID_TIMESTAMP), every one raising ERRCODE_DATETIME_FIELD_OVERFLOW, which is the same SQLSTATE 22008 as this PR's ERRCODE_DATETIME_VALUE_OUT_OF_RANGE (errcodes.h:68-69 both MAKE_SQLSTATE('2','2','0','0','8')). So every arm the PR adds is satisfied by code already in the tree. This is the exact shape CONTEXT.md names: a check that cannot go red for the change it claims to prove. The PR body's '19 passed, 3 failed' red arm is a true statement about a base that no longer exists, and neither the PR body nor the CHANGELOG entry has been re-measured against main.

Failure scenario / mutation: Apply only test/arrow_import.sh from faa313c onto main (b4f0a45) and revert 100% of this PR's src/columnar_arrow.c hunks. date_oob (INT32_MIN date32): main computes days = -2147483648-10957, IS_VALID_DATE false -> 22008. time_oob (-1 us): main's raw < 0 -> 22008. timestamp_oob (INT64_MIN us): pg_sub_s64_overflow(INT64_MIN, 946684800000000) -> 22008. timestamp_pre_min (-2^60 us): subtraction is fine, IS_VALID_TIMESTAMP(-1153868189406846976) false -> 22008. All four print PASS with the fix deleted.

2. Merging this reverts #870 and reintroduces two silent-corruption bugs (#864, #865) that are fixed in main

src/columnar_arrow.c:1541-1580 — refuter votes: stands(high) stands(high) stands(high)

This branch's imp_scalar_at is the pre-#870 code. A_DATE32 does memcpy(&v, vp, 4) unconditionally, with no date64 (width==8) arm; A_TIME64 does memcpy(&v, vp, 8) with no time32 arm and no unit scaling; A_TIMESTAMP reads raw microseconds with no unit scaling. main carries n->srcUnit (line 1350), imp_apply_field reading Date.unit/Time.unit/Time.bitWidth/Timestamp.unit (1588-1625), arrow_scale_to_usecs with floored ns narrowing (1509), and the date64 milliseconds-to-days floor (1749-1765). Because the PR conflicts in exactly these hunks, resolving toward this PR deletes all of it. OffgridwithJD measured the consequence on this branch and posted it: date64 2000-01-01 stores 4908285-05-04, timestamp('ns') 2000-01-01 stores 31969-04-01. Note also this PR's guards are strictly weaker even where they overlap: v < PG_INT64_MIN + PG_TO_UNIX_USECS is a hand-rolled special case of pg_sub_s64_overflow, and it is only correct because PG_TO_UNIX_USECS happens to be positive.

Failure scenario / mutation: Import a pandas-default file: pyarrow emits datetime64[ns] as timestamp('ns'). On main, arrow_scale_to_usecs(ARROW_TU_NANO, ...) floors ns to us and the value is correct. Resolve #862's conflict toward #862 and the same file stores 31969-04-01 with no error. Same for any date64 column: 2000-01-01 -> 4908285-05-04.

3. The blocking ask from the CHANGES_REQUESTED review is not addressed: v >= USECS_PER_DAY rejects legal time '24:00:00'

src/columnar_arrow.c:1559 — refuter votes: stands(high) stands(high) stands(high)

jdatcmd's 2026-09-01T18:19Z comment explicitly upgraded the review to BLOCKING on this and asked for > instead of >=, plus two arms (24:00:00 must SUCCEED, 24:00:00.000001 must fail 22008). Head faa313c still reads if (v < 0 || v >= USECS_PER_DAY). main already has the correct bound at line 1808: us > USECS_PER_DAY. This is not academic: export_arrow writes TimeADT verbatim as int64 (src/columnar_arrow.c:575-580 on main), so time '24:00:00' leaves our exporter as exactly 86400000000 = USECS_PER_DAY and this PR refuses to read our own file back. There is still no positive arm anywhere in the new tests: all four fixtures are extreme-negative (INT32_MIN, -1, INT64_MIN, -2^60), so the upper half of every new bound has zero coverage and nothing in the suite can see the over-rejection. docs/sql-reference.md:478-481, added by this PR, says a value 'the target type cannot represent' raises 22008 — false of 24:00:00, which the target type represents fine.

Failure scenario / mutation: CREATE TABLE t (v time) USING pgcolumnar; INSERT INTO t VALUES ('24:00:00'); SELECT pgcolumnar.export_arrow('t','/tmp/t.arrows'); then import that file back into a fresh columnar time column. On main it round-trips. With this PR it fails 22008 on data pgColumnar itself wrote.

Non-blocking

  • CHANGELOG entry asserts measurements that are no longer true of the tree it would land in (CHANGELOG.md:38-73): The entry states 'the underflow guard is the only thing between a hostile file and a stored garbage date' and 'each arm was measured red with its own guard reverted'. Both were true against merge base 8b39053 and are false against main, where pg_sub_s64_overflow (1823) already takes that input and every arm stays green with this PR's guards gone (see finding 1). The entry would also sit directly above main's existing #870 entry, which describes the same subsystem and the same 22008 behaviour from different code — a reader gets two accounts of one fix. Per CLAUDE.md's 'a correction dies when you regenerate' rule, this needs re-measuring against main, not re-wording.

@jdatcmd

jdatcmd commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Closing this as superseded by #870, with the policy question now decided by the owner.

Not closing it as wrong. This PR and #870 were written against the same real defect — the
importer read Arrow values using the target column's width and unit and ignored what the file
declared, so rows arrived silently wrong (#864/#865). Both fix it. They differ in policy:

The owner has decided on the first: convert, never refuse, and report what was lost. That is
now implemented in #880, which adds the reporting half that was genuinely missing from main.

Three things that made this the harder call than it looked, recorded so it is not relitigated:

  1. It was never only about timestamp('ns'). This PR also refuses timestamp[ms],
    date64, time32[ms] and timezone mismatches — all of which main reads correctly today
    and none of which involve any precision loss at all. Of Arrow's four timestamp units, three
    convert exactly; only nanosecond can lose anything.
  2. The conflict here is not mechanical, which is why it was left alone. This branch forks
    from 8b39053, before fix: read the temporal unit and carrier width the Arrow file declares #870. arrow_scale_to_usecs appears three times in main and zero
    times on this branch — the whole srcUnit machinery is absent. Resolving the conflict in this
    branch's favour would have deleted fix: read the temporal unit and carrier width the Arrow file declares #870 and reinstated the bug it fixed. The resolution was
    the product decision, which is why no agent took it.
  3. The loss is per value, not per type. A pandas datetime64[ns] column built from second-
    or millisecond-resolution data is nanosecond-typed and entirely lossless to convert. Refusing
    the type rejects files that convert perfectly.

What was genuinely valuable here and is NOT covered by #870: the non-temporal checks.
imp_apply_field in main inspects only Date, Time and Timestamp, with default: break for
everything else, so the file's declared Int.bitWidth/is_signed, float width,
FixedSizeBinary.byteWidth and decimal precision/scale are never read on import. A bounds check
("value buffer too small for the row count") catches the narrowing cases, so this is not a
memory-safety hole — but uint64 into bigint, a wider int into a narrower column, and
decimal128(10,2) into numeric(20,4) all have matching-or-larger carriers and would be
misread silently.

That half deserves its own issue and its own PR against current main, where it conflicts with
nothing. I have not measured those three cases, only read the code, so whoever picks it up
should start by making them fail.

Also worth stating so it is not inherited as a surprise: this branch's head deleted its own
Int.is_signed check, so its uint64 into bigint arm is currently red on its own terms.
Whatever is salvaged wants rebuilding rather than rebasing.

@jdatcmd jdatcmd closed this Sep 2, 2026
@jdatcmd

jdatcmd commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Record of the ruling this closure rests on, which was missing here.

The owner decided the policy explicitly in session, after working through always-convert-silently, refuse-the-type, lossless-or-error, convert-and-warn, and a retry loop incrementing on UNIQUE collision. His words: "After talking through it with you, I am in agreement with your recommendation. Make it happen." The recommendation was: accept nanoseconds, narrow to microseconds, never refuse, and report how many values actually lost digits — implemented in #880.

This PR is closed as superseded by that ruling plus #870, not as incorrect. The non-temporal half of its validation is genuinely uncovered by #870 and is now tracked as #881.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants