Skip to content

fix: read the temporal unit and carrier width the Arrow file declares - #870

Merged
jdatcmd merged 2 commits into
commandprompt:mainfrom
OffgridwithJD:fix/864-865-arrow-temporal
Sep 1, 2026
Merged

fix: read the temporal unit and carrier width the Arrow file declares#870
jdatcmd merged 2 commits into
commandprompt:mainfrom
OffgridwithJD:fix/864-865-arrow-temporal

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Closes #864. Closes #865.

Both issues are one root cause. import_arrow built its decode plan from the
target column type alone and never opened the Arrow Field table, so the
carrier width and the temporal unit the file declares were never read.

What was wrong

Measured on unpatched main. None of these raised an error:

file holds stored as
date64 2000-01-01 4908285-05-04
timestamp('s') 2000-01-01 1970-01-01 00:15:46.6848
timestamp('ms') 2000-01-01 1970-01-11 22:58:04.8
timestamp('ns') 2000-01-01 31969-04-01
time64('ns') 12:00:00 12000:00:00
time32('s'), time32('ms') 12:00:00 import fails, "value buffer too small"

time32 not importing at all is a third gap; neither issue mentioned it, and the
tests found it.

Only microsecond timestamps and times and date32 were correct, and those are
exactly what export_arrow writes -- so a round trip through our own exporter
never showed any of it. That is why the existing suite was green.

The fix

The Schema walk now reads Date.unit, Time.unit, Time.bitWidth and
Timestamp.unit and scales to PostgreSQL's units.

An absent field means its FlatBuffers default, and two of those defaults are
not zero.
A writer omits any field equal to its default. Measured against
pyarrow's actual bytes:

date64         unit=ABSENT   -> must mean MILLISECOND(1)
timestamp[s]   unit=ABSENT   -> must mean SECOND(0)
time32[ms]     unit=ABSENT, bitWidth=ABSENT -> MILLISECOND(1), 32
time32[s]      bitWidth=ABSENT -> 32

Reading an absent field as 0 -- the idiom used everywhere else in this file --
gives DAY for date64 and reproduces the first row of the table above. Note also
that date32/date64 share type tag 8 and time32/time64 share tag 9, so the
tag alone never settles the carrier width.

Scaling is overflow-checked (pg_mul_s64_overflow, pg_sub_s64_overflow) and
range-checked, refusing with 22008; C signed overflow is undefined behaviour,
not a wraparound. Nanoseconds narrow to microseconds, which keeps the instant
where reading ns as us is wrong by 1000x. Every narrowing floors, so an instant
before the epoch reports the day and the microsecond it falls in. Nested fields
are walked, so timestamp[] and composites with a temporal member work.

The dangerous part, and why the gate is on n->kind

n->width is both the decode stride and the divisor in imp_check_bounds, while
each non-temporal decode arm memcpys a size fixed by its kind. Taking the width
from a tag the target does not share separates the two.

My first version did exactly that, and it was a heap overread. Measured, with a
control -- a 3-row date32 file, 12-byte body:

                main            first attempt
bigint     REFUSED XX001   ACCEPTED [47064251640525, 47068546607822, 10959]
uuid       REFUSED XX001   ACCEPTED [cd2a0000-ce2a-0000-cf2a-000000000000, ...]
time       REFUSED XX001   ACCEPTED [03:02:37, 03:02:38, 03:02:39]

bigint reads 8 bytes at stride 4 off the end of the body. The walk now stamps a
node only when the file's tag matches the node's kind, and the five cross-product
arms are back to refusing.

A third bug, pre-existing, closed here

With the tag and the kind both in hand, a temporal file the target cannot hold
was being read from the low four bytes of an eight-byte carrier. Measured on
unpatched main:

time64[us] file -> date column : ACCEPTED 687342-02-27
timestamp  file -> date column : ACCEPTED 2722128-09-17
date64     file -> timestamp   : ACCEPTED 1970-01-11 22:58:04.8

These are now refused with 42804, naming both types. A non-temporal tag is
deliberately left alone, so an int64 file into a timestamp column still
imports -- there is a control arm for exactly that.

Tests

test/arrow_import.sh, 49 checks, red before green. Three premises pin that the
suite is not green-by-rejection: the exporter's own timestamp unit, its own time
unit, and date32 (which shares tag 8 with date64, so a tag-keyed fix would
break it).

Each new guarantee is proved by removal:

mutation result
recursion removed 2 red (nested list, nested struct)
absent Date.unit read as 0 2 red (both date64 arms)
kind gate removed 5 red (the whole cross product)
narrowing truncates instead of flooring 1 red
time sign checked on the scaled result 0 red -- see below

The last one is reported as it happened. Flooring already carries a negative
count to a negative microsecond count, so the raw < 0 guard reddens nothing on
its own; dropping it alone leaves 45/45 green, and only dropping it together
with flooring reddens (00000 where 22008 is wanted). It is kept as defence in
depth because it does not depend on the narrowing rule, and the code comment says
so rather than claiming it is load-bearing.

One arm I wrote had to be withdrawn: "a timestamp beyond PostgreSQL's range is
refused" is unreachable. Exceeding END_TIMESTAMP needs 9224318016000000000
microseconds, which is larger than INT64_MAX, so for every unit the overflow
guard fires first. The bounds are asymmetric; the suite asserts the reachable
(lower) one and says why.

test/arrow_corpus.py had zero temporal columns, so fuzz_arrow could not
reach any of this code. It now carries ten temporal seeds.

Gate

  • 16 suites on pg18a and pg19a: arrow_import, arrow_export,
    arrow_nested_import, fuzz_arrow, import_deferred, import_exclusion,
    import_export_privilege, export_sink, entry_point_privilege,
    row_triggers, rls_direct_storage, server_file_privilege,
    local_open_race_free, native_parquet_units, parquet_import,
    parquet_export -- 16/16 both.
  • The four Arrow suites on pg15a, pg16a, pg17a, pg18a, pg19a -- all green.
  • ASAN+UBSAN via test/run_san.sh (arrow_import is in its subset).

Docs and CHANGELOG ship with the change.

Overlap with #861 and #862 — please read before merging any of the three

All three touch src/columnar_arrow.c and test/arrow_import.sh, so textual
conflicts are certain. They are not alternatives to each other in equal measure:

The temporal tag/kind cross-check here is a narrow, temporal-only case of what
#861 does generally. If #861 lands first I will rebase onto it and drop whatever
it already covers, keeping the unit decoding, the FlatBuffers default handling,
the narrowing rule, and the nested walk. If this lands first, #861 should keep
its non-temporal validation and drop any arm that refuses a well-formed temporal
file.

I have not re-measured #861's and #862's current behaviour on their present heads
in this session, so treat the two bullets above as a reading of their stated
scope, not as a measurement of their code. Sequencing is the maintainer's call; I
am flagging the interaction, not asserting a merge order.

import_arrow built its decode plan from the target column type alone and never
opened the Arrow Field table, so the file's carrier width and temporal unit were
never read. One root cause, three symptoms (commandprompt#864, commandprompt#865).

Measured on unpatched main, all silent:

    date64        2000-01-01 -> 4908285-05-04
    timestamp(s)  2000-01-01 -> 1970-01-01 00:15:46.6848
    timestamp(ms) 2000-01-01 -> 1970-01-11 22:58:04.8
    timestamp(ns) 2000-01-01 -> 31969-04-01
    time64(ns)    12:00:00   -> 12000:00:00
    time32(s|ms)  12:00:00   -> import fails, "value buffer too small"

Only the microsecond timestamp and time units and date32 were read correctly,
and those are exactly what export_arrow writes, so a round trip through our own
exporter never showed it.

The Schema walk now reads Date.unit, Time.unit, Time.bitWidth and
Timestamp.unit, and scales to PostgreSQL's units. An absent field means its
FlatBuffers default, and two of those defaults are not zero: measured against
pyarrow, date64 and time32[ms] carry no unit field at all. Reading an absent
field as 0 is what produced the date64 result above.

Scaling is overflow-checked with pg_mul_s64_overflow / pg_sub_s64_overflow and
range-checked, refusing with 22008 rather than wrapping; C signed overflow is
undefined behaviour, not a wraparound. Nanoseconds narrow to microseconds, which
keeps the instant where reading ns as us is wrong by 1000x. Every narrowing
floors, so an instant before the epoch reports the day and microsecond it falls
in. Nested fields are walked too, so a timestamp[] or a composite with a
temporal member is read by its own unit.

n->width is both the decode stride and the divisor in imp_check_bounds, while
each non-temporal decode arm reads a size fixed by its kind. A width taken from
a tag the target does not share separates the two, so the walk stamps a node
only when the file's tag matches the node's kind. Without that gate a
Date-tagged field set width 4 under an A_INT64 node that still reads 8 bytes,
passing the bounds check and reading off the end of the body.

That check also closes a third case, present since Arrow import shipped: a
temporal file the target column cannot hold was read anyway from the low four
bytes of an eight-byte carrier. Measured on main, a time64 file into a date
column stored 687342-02-27 and a timestamp file into one stored 2722128-09-17.
Both are now refused with 42804. A non-temporal tag is left alone, so an int64
file into a timestamp column still imports.

test/arrow_corpus.py had no temporal columns at all, so fuzz_arrow could not
reach any of this; it now carries ten temporal seeds.

Tests: test/arrow_import.sh 49 checks, red before green, each new guarantee
proved by removal. The raw-sign guard on time is reported honestly as redundant
under flooring and kept as defence in depth.

Gate: arrow_import, arrow_export, arrow_nested_import, fuzz_arrow,
import_deferred, import_exclusion, import_export_privilege, export_sink,
entry_point_privilege, row_triggers, rls_direct_storage, server_file_privilege,
local_open_race_free, native_parquet_units, parquet_import, parquet_export --
16/16 on pg18a and pg19a; the four Arrow suites green on pg15a..pg19a.
test/docs_style.sh measures sentence length and refuses anything over 25 words.
Two sentences added to docs/sql-reference.md in the previous commit were 32 and
26 words, and the suite matrix caught them on PG17 and PG18.

They say the same thing in three sentences and two.

@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 the current head. Approving. I ran the mutations rather than trusting the body, and the suite catches the fix being reverted.

The suite is falsifiable, which is the thing I could not take on faith

Two mutations of the scaling itself, 49 checks on every arm, pg18_assert:

baseline                                    49 checks   0 red
second-to-microsecond scale -> no-op        49 checks   3 red
  FAIL  a timestamp in seconds decodes to the instant it holds (#865)
        got [1970-01-01 00:15:46.6848] want [2000-01-01 00:00:00]
  FAIL  a time32 in seconds decodes to the time it holds (#865)
  FAIL  a second count that overflows on scaling is refused, not wrapped  got [00000] want [22008]
millisecond scale -> no-op                  49 checks   3 red
  FAIL  a timestamp in milliseconds decodes to the instant it holds (#865)
        got [1970-01-11 22:58:04.8] want [2000-01-01 00:00:00]
  FAIL  a time32 in milliseconds decodes to the time it holds (#865)
  FAIL  a timestamp before PostgreSQL's range is refused  got [00000] want [22008]

The reverted values reproduce your table exactly — 1970-01-01 00:15:46.6848 and 1970-01-11 22:58:04.8 — so the arms are pinned to the real behaviour and not to a restatement of it. The overflow arms redden too, which means the pg_mul_s64_overflow path is covered rather than merely present.

The FlatBuffers defaults are right here, and that is not a given

Date.unit defaults to MILLISECOND, Time to MILLISECOND/32, Timestamp.unit to SECOND — an absent field means the schema's default, and two of those are non-zero. You state that in the body and the code follows it.

Worth saying because #861 gets the same class wrong: its A_FLOAT64 arm uses 2 (DOUBLE) as the default for an absent FloatingPoint.precision, where Arrow's is HALF (0). Same file, same kind of question, opposite outcome. Whichever of these lands second should re-check the other.

And it fixes the boundary #862 gets wrong

#870  us > USECS_PER_DAY     accepts time '24:00:00'   correct
#862  v >= USECS_PER_DAY     rejects it                 breaks a legal value

time '24:00:00' is legal PostgreSQL and equals USECS_PER_DAY exactly; 24:00:00.000001 is the first invalid one. This PR has it right. I have requested changes on #862 for the same expression.

Two MINOR points, neither blocking

The boundary itself is untested. Nothing in the new suite imports time '24:00:00' or the microsecond above it. The bound is correct today and no arm would notice it drifting to >= — which is precisely the mistake sitting in a sibling PR right now. Two fixtures close it.

Third gap found by the tests, not by the issues. time32('s') and time32('ms') did not import at all — "value buffer too small" — and neither #864 nor #865 mentions it. That is the tests doing their job and it is worth keeping in the changelog entry as such.

Why this one merges and eleven others do not

It is the only non-draft, and it earns it: measured problem statement, correct defaults, overflow-checked scaling, 49 checks that redden when the fix is removed. CI green.

@jdatcmd
jdatcmd merged commit 53224e4 into commandprompt:main Sep 1, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants