From e8341ee75e17f54dd5a997fc9e3b647eae818e3b Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 2 Sep 2026 12:29:35 -0600 Subject: [PATCH 1/3] fix: report the nanosecond narrowing instead of doing it silently pgcolumnar.import_arrow narrows Arrow nanoseconds to PostgreSQL microseconds and always has. PostgreSQL timestamps and times are int64 microseconds; Arrow parameterises the unit in the type. Of Arrow's four units, second, millisecond and microsecond all widen or match exactly, so only nanosecond can lose anything -- and only for a value not already on a microsecond boundary. The narrowing was silent. An import that changed the data was indistinguishable from one that did not, which matters more than precision alone: truncation to the microsecond can make rows that were DISTINCT in the file compare EQUAL here, so a UNIQUE violation or a lost ORDER BY tie-break surfaced later with nothing connecting it to its cause. It now ends with NOTICE: columnar.import_arrow: 5 of 10 values lost sub-microsecond precision COUNTED PER VALUE, NOT PER TYPE. This is the whole design. A pandas datetime64[ns] column built from second- or millisecond-resolution data is nanosecond-TYPED and entirely lossless to convert, and reports nothing. Refusing the TYPE -- the alternative that was on the table -- would reject files that convert perfectly, including the default output of the commonest producer. NOTHING IS REFUSED. NOTICE, not WARNING or ERROR: a bulk load must not fail on the last row of a large file for a conversion the caller may well have intended. Out-of-range values are still refused with 22008, unchanged, and the floor rounding is unchanged. The counter costs an increment. arrow_floordiv already computes v % 1000 to decide the rounding direction; the remainder was being discarded. Threaded through imp_value_at/imp_scalar_at rather than held in a file-scope static: this file has no mutable global state and should not acquire its first one for a counter. Red before green: the report arm fails on unmodified main, "got [0] want [1]". Removal proof, one .so per arm, each mutation asserted applied: unmutated 56 passed + 0 failed never count 55 passed + 1 failed count but never report 55 passed + 1 failed Both halves are load-bearing and only the report arm moves. The two controls -- an ns-TYPED file whose values are all on microsecond boundaries, and a microsecond file -- stay green under both mutations, so the counter measures the loss rather than the unit. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017V7PhZ1TzoVVNsACFXTbdT --- CHANGELOG.md | 33 +++++++++++++++++++ src/columnar_arrow.c | 59 ++++++++++++++++++++++++++++------ test/arrow_import.sh | 76 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1742a1c..b8c568f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,39 @@ true until the next version shipped. ### Added +- A nanosecond Arrow import says how many values lost precision, and still + imports every row. + + **`pgcolumnar.import_arrow` narrows nanoseconds to microseconds and always + did.** PostgreSQL timestamps and times are int64 microseconds; Arrow + parameterises the unit in the type. Of Arrow's four units, second, + millisecond and microsecond all widen or match exactly, so only nanosecond can + lose anything, and only for a value that is not already on a microsecond + boundary. + + The narrowing was silent, and that is what changed. An import now ends with + + NOTICE: columnar.import_arrow: 5 of 10 values lost sub-microsecond precision + DETAIL: PostgreSQL timestamps and times hold microseconds; this Arrow file + declares nanoseconds. + HINT: Import the raw nanoseconds into a bigint column if the extra digits + are significant. + + **Counted 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; it reports nothing. Only values that actually carried + sub-microsecond digits are counted. The remainder is not extra work: + `arrow_floordiv` already computes it to decide the rounding direction. + + **Nothing is refused.** `NOTICE`, not `WARNING` or `ERROR`: a bulk load must + not fail on the last row of a large file for a conversion the caller may have + intended. Out-of-range values are still refused with `22008`, unchanged. + + Truncation does more than reduce precision -- it can make rows that were + distinct in the file compare EQUAL here, so a `UNIQUE` violation or a lost + `ORDER BY` tie-break now has its cause stated at the point it was introduced + rather than surfacing later as a mystery. + - The two visibility-map clears that no test held are now held (#877). **Three paths retire a row group and give its live rows new row numbers**, and diff --git a/src/columnar_arrow.c b/src/columnar_arrow.c index e239e449..69bdb60c 100644 --- a/src/columnar_arrow.c +++ b/src/columnar_arrow.c @@ -1505,8 +1505,22 @@ arrow_floordiv(int64 num, int64 den) return q; } +/* + * Scale an Arrow temporal value onto PostgreSQL's microseconds. + * + * Of the four Arrow units, second, millisecond and microsecond all widen or + * match, so only nanosecond can lose anything -- and only for a value that is + * not already on a microsecond boundary. A pandas datetime64[ns] column built + * from second- or millisecond-resolution data is nanosecond-TYPED and entirely + * lossless to convert, which is why the loss is counted per VALUE and not + * refused per TYPE. + * + * nsTrunc, when not NULL, is incremented once per value that actually lost + * digits. The remainder is not extra work: arrow_floordiv already computes it + * to decide the rounding direction. + */ static bool -arrow_scale_to_usecs(int unit, int64 v, int64 *out) +arrow_scale_to_usecs(int unit, int64 v, int64 *out, int64 *nsTrunc) { switch (unit) { @@ -1515,6 +1529,8 @@ arrow_scale_to_usecs(int unit, int64 v, int64 *out) case ARROW_TU_MILLI: return !pg_mul_s64_overflow(v, INT64CONST(1000), out); case ARROW_TU_NANO: + if (nsTrunc != NULL && v % INT64CONST(1000) != 0) + (*nsTrunc)++; *out = arrow_floordiv(v, INT64CONST(1000)); return true; case ARROW_TU_MICRO: @@ -1654,7 +1670,7 @@ imp_apply_field(ImpNode *n, const uint8 *meta, uint32 metaLen, uint32 field) static Datum imp_scalar_at(ImpNode *n, const uint8 *body, const int64 *bufOff, - const int64 *bufLen, int64 i) + const int64 *bufLen, int64 i, int64 *nsTrunc) { if (n->kind == A_BOOL) { @@ -1804,7 +1820,7 @@ imp_scalar_at(ImpNode *n, const uint8 *body, const int64 *bufOff, * midnight for a malformed input. */ if (raw < INT64CONST(0) || - !arrow_scale_to_usecs(n->srcUnit, raw, &us) || + !arrow_scale_to_usecs(n->srcUnit, raw, &us, nsTrunc) || us > USECS_PER_DAY) ereport(ERROR, (errcode(ERRCODE_DATETIME_FIELD_OVERFLOW), @@ -1819,7 +1835,7 @@ imp_scalar_at(ImpNode *n, const uint8 *body, const int64 *bufOff, int64 t; memcpy(&raw, vp, 8); - if (!arrow_scale_to_usecs(n->srcUnit, raw, &us) || + if (!arrow_scale_to_usecs(n->srcUnit, raw, &us, nsTrunc) || pg_sub_s64_overflow(us, PG_TO_UNIX_USECS, &t) || !IS_VALID_TIMESTAMP((Timestamp) t)) ereport(ERROR, @@ -1859,7 +1875,7 @@ imp_scalar_at(ImpNode *n, const uint8 *body, const int64 *bufOff, /* reconstruct a node's value at index i (recursively for list/struct) */ static Datum imp_value_at(ImpNode *n, const uint8 *body, const int64 *bufOff, - const int64 *bufLen, int64 i, bool *isnull) + const int64 *bufLen, int64 i, bool *isnull, int64 *nsTrunc) { *isnull = imp_is_null(body, bufOff[n->validBuf], bufLen[n->validBuf], i); if (*isnull) @@ -1885,7 +1901,7 @@ imp_value_at(ImpNode *n, const uint8 *body, const int64 *bufOff, enulls = (nelem > 0) ? palloc(sizeof(bool) * nelem) : NULL; for (k = 0; k < nelem; k++) elems[k] = imp_value_at(&n->children[0], body, bufOff, bufLen, - start + k, &enulls[k]); + start + k, &enulls[k], nsTrunc); dims[0] = nelem; arr = construct_md_array(elems, enulls, 1, dims, lbs, n->elemtype, n->elemlen, n->elembyval, n->elemalign); @@ -1907,12 +1923,13 @@ imp_value_at(ImpNode *n, const uint8 *body, const int64 *bufOff, fn[a] = true; continue; } - fv[a] = imp_value_at(&n->children[ci++], body, bufOff, bufLen, i, &fn[a]); + fv[a] = imp_value_at(&n->children[ci++], body, bufOff, bufLen, i, + &fn[a], nsTrunc); } tup = heap_form_tuple(n->structDesc, fv, fn); return HeapTupleGetDatum(tup); } - return imp_scalar_at(n, body, bufOff, bufLen, i); + return imp_scalar_at(n, body, bufOff, bufLen, i, nsTrunc); } /* @@ -2036,6 +2053,7 @@ pgcolumnar_import_arrow(PG_FUNCTION_ARGS) CommandId cid; MemoryContext rowCtx; int64 total = 0; + int64 nsTrunc = 0; /* values that lost sub-microsecond digits */ bool sawSchema = false; int i; @@ -2267,7 +2285,8 @@ pgcolumnar_import_arrow(PG_FUNCTION_ARGS) bool isnull; slot->tts_values[i] = imp_value_at(&tops[i], body, bufOff, - bufLen, r, &isnull); + bufLen, r, &isnull, + &nsTrunc); slot->tts_isnull[i] = isnull; } ExecStoreVirtualTuple(slot); @@ -2319,5 +2338,27 @@ pgcolumnar_import_arrow(PG_FUNCTION_ARGS) * INSERT. */ table_close(rel, NoLock); + + /* + * Report the narrowing rather than refuse it. An import that changed the + * data must not be indistinguishable from one that did not: truncation to + * the microsecond can make rows that were distinct in the file EQUAL here, + * so a later UNIQUE violation or a lost ORDER BY tie-break has its cause + * stated where it was introduced instead of surfacing as a mystery. + * + * NOTICE, not WARNING or ERROR: nothing is malformed and nothing is + * refused, and a bulk load must not die on the last row of a large file for + * a conversion the caller may well have intended. Counted per VALUE, so a + * nanosecond-TYPED file whose values all sit on microsecond boundaries -- + * which is what pandas produces from second- or millisecond-resolution data + * -- says nothing at all. + */ + if (nsTrunc > 0) + ereport(NOTICE, + (errmsg("columnar.import_arrow: " INT64_FORMAT " of " INT64_FORMAT + " values lost sub-microsecond precision", nsTrunc, total), + errdetail("PostgreSQL timestamps and times hold microseconds; this Arrow file declares nanoseconds."), + errhint("Import the raw nanoseconds into a bigint column if the extra digits are significant."))); + PG_RETURN_INT64(total); } diff --git a/test/arrow_import.sh b/test/arrow_import.sh index 56d2e3d8..25f7aea1 100755 --- a/test/arrow_import.sh +++ b/test/arrow_import.sh @@ -126,6 +126,82 @@ check "non-finite imported as null" "$nulls" "1" keep="$(q "SELECT count(*) FROM ri_nf2 WHERE id=2 AND dt='2020-01-01' AND num=1.25;")" check "finite row preserved" "$keep" "1" +# --- 3b. documented lossy mapping: nanosecond -> microsecond, and it is REPORTED +# +# PostgreSQL timestamps and times are int64 MICROSECONDS; Arrow parameterises the +# unit in the type. Of the four Arrow units, second, millisecond and microsecond +# all widen or match exactly, so only nanosecond can lose anything -- and only for +# values that are not already on a microsecond boundary. A pandas datetime64[ns] +# column built from second- or millisecond-resolution data is nanosecond-TYPED and +# entirely lossless to convert. +# +# The contract is: never refuse over this. Narrow it, keep every row, and say how +# many values actually lost digits. Silence would make an import that changed the +# data indistinguishable from one that did not -- and truncation does more than +# reduce precision, it can make distinct rows EQUAL, which is what a later UNIQUE +# violation would be reporting without explaining. +# +# The two controls are the point of the section: an ns-typed file whose values are +# all on a microsecond boundary must report NOTHING, and a microsecond file must +# report nothing, or the counter is measuring the unit rather than the loss. +if [ "$have_pyarrow" = 1 ]; then + echo "-- nanosecond narrowing is reported, never refused" + psql_c() { env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" \ + -U postgres -d "$PGC_DB" -At -c "$1" 2>&1; } + + NSLOSSY="$PGC_WORKDIR/ns_lossy.arrows" + NSEXACT="$PGC_WORKDIR/ns_exact.arrows" + USPLAIN="$PGC_WORKDIR/us_plain.arrows" + python3 - "$NSLOSSY" "$NSEXACT" "$USPLAIN" <<'PY' +import sys, pyarrow as pa, pyarrow.ipc as ipc +S2000 = 946684800 +def w(path, arr): + t = pa.table({'ts': arr}) + with ipc.new_stream(pa.OSFile(path, 'wb'), t.schema) as o: + o.write_table(t) +# every value carries 123ns past a microsecond boundary: all four truncate +w(sys.argv[1], pa.array([(S2000 + i) * 10**9 + 123 for i in range(4)], pa.timestamp('ns'))) +# nanosecond-TYPED but microsecond-exact: nothing is lost, nothing to report +w(sys.argv[2], pa.array([(S2000 + i) * 10**9 for i in range(4)], pa.timestamp('ns'))) +# a different unit entirely: the counter must not fire on it +w(sys.argv[3], pa.array([(S2000 + i) * 10**6 for i in range(4)], pa.timestamp('us'))) +PY + + psql_run "CREATE TABLE ri_nsl (ts timestamp) USING pgcolumnar;" + psql_run "CREATE TABLE ri_nse (ts timestamp) USING pgcolumnar;" + psql_run "CREATE TABLE ri_usp (ts timestamp) USING pgcolumnar;" + + nsl_out="$(psql_c "SELECT pgcolumnar.import_arrow('ri_nsl', '$NSLOSSY');")" + nse_out="$(psql_c "SELECT pgcolumnar.import_arrow('ri_nse', '$NSEXACT');")" + usp_out="$(psql_c "SELECT pgcolumnar.import_arrow('ri_usp', '$USPLAIN');")" + + # THE CONTRACT: not one row refused, on any of the three. + check "a lossy nanosecond file imports every row" \ + "$(q "SELECT count(*) FROM ri_nsl;")" "4" + check "so does a microsecond-exact nanosecond file" \ + "$(q "SELECT count(*) FROM ri_nse;")" "4" + check "and a microsecond file" \ + "$(q "SELECT count(*) FROM ri_usp;")" "4" + + # THE ARM: the loss is counted and reported, once, with the number. + check "the narrowing is reported, and names how many values lost digits" \ + "$(printf '%s' "$nsl_out" | grep -c 'NOTICE.*4 .*sub-microsecond')" "1" + + # THE CONTROLS: no report when nothing was lost. + check "control: an ns file on microsecond boundaries reports nothing" \ + "$(printf '%s' "$nse_out" | grep -ci 'sub-microsecond')" "0" + check "control: a microsecond file reports nothing" \ + "$(printf '%s' "$usp_out" | grep -ci 'sub-microsecond')" "0" + + # The conversion itself is unchanged: floor to the microsecond, not rounded + # and not refused. 123ns past the boundary lands ON the boundary. + check "the narrowed values are floored to the microsecond" \ + "$(q "SELECT string_agg(ts::text, ',' ORDER BY ts) FROM ri_nsl;")" \ + "2000-01-01 00:00:00,2000-01-01 00:00:01,2000-01-01 00:00:02,2000-01-01 00:00:03" +else + echo "-- pyarrow not available; skipping nanosecond narrowing checks" +fi + # --- 4. error cases --------------------------------------------------------- echo "-- argument validation" psql_run "CREATE TABLE ri_heap (a bigint, b float8, c text) USING heap;" From 313ae01f95fc052fbd7629c0f908ba2251b3f9a3 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 2 Sep 2026 13:26:26 -0600 Subject: [PATCH 2/3] fix: the NOTICE counts values, so it must not divide them by rows (#880 review) The message read "N of M values", where N counted temporal VALUES that lost digits and M counted ROWS imported. Three timestamp('ns') columns over four rows printed NOTICE: columnar.import_arrow: 12 of 4 values lost sub-microsecond precision Both numbers are true and the sentence joining them is not, in the one message whose entire purpose is to inform the operator. Reproduced before fixing. The denominator is dropped rather than corrected. A right one would need a second counter for temporal values CONVERTED, and the count alone is what the reader acts on. The comment now says so, so nobody helpfully adds a denominator back. AND THE ARM COULD NOT HAVE CAUGHT IT, which is the more important half. It was grep -c 'NOTICE.*4 .*sub-microsecond' want 1 and every fixture had exactly ONE temporal column, so the value count and the row count were both 4. The "4" being matched was the DENOMINATOR. A build whose counter reported an entirely different population passed it. One fixture fixes both: two temporal columns over four rows makes the value count 8 while the row count stays 4, so the two can be told apart. The arm now pins the exact string "8 values lost sub-microsecond precision" and a second arm asserts the row count 4 is NOT what gets reported. Removal proof re-run on the corrected suite, one .so per arm, each mutation asserted applied: unmutated 57 passed + 0 failed count at most one value 56 passed + 1 failed never count 56 passed + 1 failed count but never report 56 passed + 1 failed The first mutation is the reviewer's, and it scored 56 passed + 0 failed against the old arm. It reddens now. Not changed, and deliberately: the message says columnar.import_arrow rather than pgcolumnar.import_arrow. Seven existing errmsgs in this file already say columnar.import_arrow / columnar.export_arrow; matching them is right and renaming all eight is not this change's job. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017V7PhZ1TzoVVNsACFXTbdT --- CHANGELOG.md | 2 +- src/columnar_arrow.c | 10 ++++++++-- test/arrow_import.sh | 37 +++++++++++++++++++++++++------------ 3 files changed, 34 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8c568f3..2dbd107c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,7 @@ true until the next version shipped. The narrowing was silent, and that is what changed. An import now ends with - NOTICE: columnar.import_arrow: 5 of 10 values lost sub-microsecond precision + NOTICE: columnar.import_arrow: 12 values lost sub-microsecond precision DETAIL: PostgreSQL timestamps and times hold microseconds; this Arrow file declares nanoseconds. HINT: Import the raw nanoseconds into a bigint column if the extra digits diff --git a/src/columnar_arrow.c b/src/columnar_arrow.c index 69bdb60c..a6b952aa 100644 --- a/src/columnar_arrow.c +++ b/src/columnar_arrow.c @@ -2352,11 +2352,17 @@ pgcolumnar_import_arrow(PG_FUNCTION_ARGS) * nanosecond-TYPED file whose values all sit on microsecond boundaries -- * which is what pandas produces from second- or millisecond-resolution data * -- says nothing at all. + * + * No denominator. nsTrunc counts VALUES and total counts ROWS, so "N of M" + * divides one population by another: three nanosecond columns over four rows + * printed "12 of 4 values", both numbers true and the sentence joining them + * false. A correct denominator would need a second counter for temporal + * values converted, and the count alone is what the reader acts on. */ if (nsTrunc > 0) ereport(NOTICE, - (errmsg("columnar.import_arrow: " INT64_FORMAT " of " INT64_FORMAT - " values lost sub-microsecond precision", nsTrunc, total), + (errmsg("columnar.import_arrow: " INT64_FORMAT + " values lost sub-microsecond precision", nsTrunc), errdetail("PostgreSQL timestamps and times hold microseconds; this Arrow file declares nanoseconds."), errhint("Import the raw nanoseconds into a bigint column if the extra digits are significant."))); diff --git a/test/arrow_import.sh b/test/arrow_import.sh index 25f7aea1..0da9ef49 100755 --- a/test/arrow_import.sh +++ b/test/arrow_import.sh @@ -155,21 +155,27 @@ if [ "$have_pyarrow" = 1 ]; then python3 - "$NSLOSSY" "$NSEXACT" "$USPLAIN" <<'PY' import sys, pyarrow as pa, pyarrow.ipc as ipc S2000 = 946684800 -def w(path, arr): - t = pa.table({'ts': arr}) +def w(path, a, b): + t = pa.table({'ts': a, 'ts2': b}) with ipc.new_stream(pa.OSFile(path, 'wb'), t.schema) as o: o.write_table(t) -# every value carries 123ns past a microsecond boundary: all four truncate -w(sys.argv[1], pa.array([(S2000 + i) * 10**9 + 123 for i in range(4)], pa.timestamp('ns'))) +# TWO temporal columns over four rows, so the number of VALUES that lose digits +# (8) differs from the number of ROWS (4). With one column the two coincide and +# an arm cannot tell which one the message is reporting. +def ns(off): + return pa.array([(S2000 + i) * 10**9 + off for i in range(4)], pa.timestamp('ns')) +# every value carries 123ns past a microsecond boundary: all EIGHT truncate +w(sys.argv[1], ns(123), ns(123)) # nanosecond-TYPED but microsecond-exact: nothing is lost, nothing to report -w(sys.argv[2], pa.array([(S2000 + i) * 10**9 for i in range(4)], pa.timestamp('ns'))) +w(sys.argv[2], ns(0), ns(0)) # a different unit entirely: the counter must not fire on it -w(sys.argv[3], pa.array([(S2000 + i) * 10**6 for i in range(4)], pa.timestamp('us'))) +us = lambda: pa.array([(S2000 + i) * 10**6 for i in range(4)], pa.timestamp('us')) +w(sys.argv[3], us(), us()) PY - psql_run "CREATE TABLE ri_nsl (ts timestamp) USING pgcolumnar;" - psql_run "CREATE TABLE ri_nse (ts timestamp) USING pgcolumnar;" - psql_run "CREATE TABLE ri_usp (ts timestamp) USING pgcolumnar;" + psql_run "CREATE TABLE ri_nsl (ts timestamp, ts2 timestamp) USING pgcolumnar;" + psql_run "CREATE TABLE ri_nse (ts timestamp, ts2 timestamp) USING pgcolumnar;" + psql_run "CREATE TABLE ri_usp (ts timestamp, ts2 timestamp) USING pgcolumnar;" nsl_out="$(psql_c "SELECT pgcolumnar.import_arrow('ri_nsl', '$NSLOSSY');")" nse_out="$(psql_c "SELECT pgcolumnar.import_arrow('ri_nse', '$NSEXACT');")" @@ -183,9 +189,16 @@ PY check "and a microsecond file" \ "$(q "SELECT count(*) FROM ri_usp;")" "4" - # THE ARM: the loss is counted and reported, once, with the number. - check "the narrowing is reported, and names how many values lost digits" \ - "$(printf '%s' "$nsl_out" | grep -c 'NOTICE.*4 .*sub-microsecond')" "1" + # THE ARM: the loss is counted and reported, once, with the number -- and the + # number is the count of VALUES (8), not of rows (4). The fixture has two + # temporal columns precisely so those two differ: with one column they + # coincide, and an arm matching "4" is satisfied by either, which is how an + # earlier revision of this suite passed a build whose counter reported the + # wrong population entirely. + check "the narrowing reports the number of VALUES that lost digits, not rows" \ + "$(printf '%s' "$nsl_out" | grep -c 'columnar.import_arrow: 8 values lost sub-microsecond precision')" "1" + check "and does not report the row count instead" \ + "$(printf '%s' "$nsl_out" | grep -c 'import_arrow: 4 values lost')" "0" # THE CONTROLS: no report when nothing was lost. check "control: an ns file on microsecond boundaries reports nothing" \ From 6a23274789cdea203c458ae7815651bfee37c79b Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 2 Sep 2026 17:13:41 -0600 Subject: [PATCH 3/3] test: cover the three paths to the counter that no arm reached (#880) Self-review, applying the reviewer's own question to my own work: what would still pass if this stopped working? The counter lives in arrow_scale_to_usecs, which is reached from TWO call sites -- the time arm at columnar_arrow.c:1823 and the timestamp arm at :1838 -- and is threaded through the list recursion at :1904 and the struct recursion at :1927. Every arm in the previous revision used a single top-level timestamp('ns') column. So passing NULL for nsTrunc at the time site, or dropping it from the nested recursion, moved nothing red. One fixture covers them: a time64('ns') column (4 values) and a list of two elements per row (8 values), all 123ns past a microsecond boundary, so the file reports 12 -- a number that is neither the row count nor the other fixture's count. Removal proof, one .so per arm, each mutation asserted applied: unmutated 59 passed + 0 failed NULL for nsTrunc at the TIME site 58 passed + 1 failed only the new arm NULL for nsTrunc in the LIST recursion 58 passed + 1 failed only the new arm Both leave the top-level timestamp arm green, so the new arm covers exactly the paths the old ones missed rather than duplicating them. The counter was already correct on all three paths -- this adds no production code. What it adds is the ability to notice if that stops being true. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017V7PhZ1TzoVVNsACFXTbdT --- CHANGELOG.md | 4 ++++ test/arrow_import.sh | 28 +++++++++++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2dbd107c..f88f82c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,10 @@ true until the next version shipped. sub-microsecond digits are counted. The remainder is not extra work: `arrow_floordiv` already computes it to decide the rounding direction. + The counter is reached from the time arm as well as the timestamp arm, and is + threaded through the list and struct recursion, so a nested `timestamp[]` and a + `time64[ns]` column are counted the same way. + **Nothing is refused.** `NOTICE`, not `WARNING` or `ERROR`: a bulk load must not fail on the last row of a large file for a conversion the caller may have intended. Out-of-range values are still refused with `22008`, unchanged. diff --git a/test/arrow_import.sh b/test/arrow_import.sh index 0da9ef49..2ca09070 100755 --- a/test/arrow_import.sh +++ b/test/arrow_import.sh @@ -152,7 +152,8 @@ if [ "$have_pyarrow" = 1 ]; then NSLOSSY="$PGC_WORKDIR/ns_lossy.arrows" NSEXACT="$PGC_WORKDIR/ns_exact.arrows" USPLAIN="$PGC_WORKDIR/us_plain.arrows" - python3 - "$NSLOSSY" "$NSEXACT" "$USPLAIN" <<'PY' + NSDEEP="$PGC_WORKDIR/ns_deep.arrows" + python3 - "$NSLOSSY" "$NSEXACT" "$USPLAIN" "$NSDEEP" <<'PY' import sys, pyarrow as pa, pyarrow.ipc as ipc S2000 = 946684800 def w(path, a, b): @@ -171,6 +172,21 @@ w(sys.argv[2], ns(0), ns(0)) # a different unit entirely: the counter must not fire on it us = lambda: pa.array([(S2000 + i) * 10**6 for i in range(4)], pa.timestamp('us')) w(sys.argv[3], us(), us()) +# The counter is reached from TWO call sites -- the time arm and the timestamp +# arm -- and is threaded through the list and struct recursion. A file with only +# a top-level timestamp column leaves the other three paths unmeasured: passing +# NULL at the time site, or dropping the counter from the nested recursion, +# would not move a single arm above. This file covers them. +# time64[ns] 4 values, all 123ns past a boundary +# list x2 8 values, likewise +# 12 total +NOON = 12 * 3600 +tm = pa.array([(NOON + i) * 10**9 + 123 for i in range(4)], pa.time64('ns')) +lst = pa.array([[(S2000 + i) * 10**9 + 123, (S2000 + i + 1) * 10**9 + 123] + for i in range(4)], pa.list_(pa.timestamp('ns'))) +deep = pa.table({'tm': tm, 'lst': lst}) +with ipc.new_stream(pa.OSFile(sys.argv[4], 'wb'), deep.schema) as o: + o.write_table(deep) PY psql_run "CREATE TABLE ri_nsl (ts timestamp, ts2 timestamp) USING pgcolumnar;" @@ -200,6 +216,16 @@ PY check "and does not report the row count instead" \ "$(printf '%s' "$nsl_out" | grep -c 'import_arrow: 4 values lost')" "0" + # The other three paths that reach the counter. Without this arm, passing + # NULL for nsTrunc at the time call site, or dropping it from the list or + # struct recursion, leaves every arm above green. + psql_run "CREATE TABLE ri_nsd (tm time, lst timestamp[]) USING pgcolumnar;" + nsd_out="$(psql_c "SELECT pgcolumnar.import_arrow('ri_nsd', '$NSDEEP');")" + check "a time64 column and a nested list are counted too, not just a top-level timestamp" \ + "$(printf '%s' "$nsd_out" | grep -c 'columnar.import_arrow: 12 values lost sub-microsecond precision')" "1" + check "and that file imports every row as well" \ + "$(q "SELECT count(*) FROM ri_nsd;")" "4" + # THE CONTROLS: no report when nothing was lost. check "control: an ns file on microsecond boundaries reports nothing" \ "$(printf '%s' "$nse_out" | grep -ci 'sub-microsecond')" "0"