GH-600: Allow TimestampType to annotate FLBA(12) - #601
Open
divjotarora wants to merge 1 commit into
Open
Conversation
stevomitric
approved these changes
Jul 21, 2026
alkis
reviewed
Jul 27, 2026
| since the Unix epoch. | ||
|
|
||
| For the `FIXED_LEN_BYTE_ARRAY` carrier (with `type_length = 12`), the value is a | ||
| signed 96-bit two's-complement little-endian integer count of `unit`s since the |
Contributor
There was a problem hiding this comment.
I suggest we do big-endian here so that we can use the same signed two complements byte compare we use for DECIMAL in FLBA.
Contributor
Author
There was a problem hiding this comment.
I chose little-endian because it seems to match the rest of the spec (DECIMAL is the only deviation). But I don't have a strong preference, happy to change it to big-endian. I'd like to hear from others to see if anyone else agrees/disagrees.
emkornfield
approved these changes
Aug 6, 2026
emkornfield
left a comment
emkornfield
left a comment
Contributor
There was a problem hiding this comment.
LGTM, we can bikeshed more on little endian vs big-endian to finalize this.
CurtHagenlocher
added a commit
to clast-project/engineered-wood
that referenced
this pull request
Aug 22, 2026
…ncated bounds, an ungated annotation (#215) * fix(parquet): gate the TIMESTAMP annotation on its physical type Two pre-existing defects, both latent behind the same assumption: that TIMESTAMP only ever arrives on INT64. apache/parquet-format#601 is about to end that, so they stop being theoretical. THE READ DEFECT. `ArrowSchemaConverter.FromLogicalType` mapped `TimestampType` to an Arrow `TimestampType` without looking at `column.PhysicalType` -- unlike `MakeDecimalType`, which has always switched on it. The read path maps `Int64Type or TimestampType or Time64Type` onto a `long` value buffer, so a TIMESTAMP annotation on a 12-byte column was reinterpreted eight bytes at a time and produced plausible-looking wrong dates. Not an error, not a refusal -- silently wrong data, which is the worst of the three. The same hole existed on the converted-type path, where TIMESTAMP_MILLIS / TIMESTAMP_MICROS are likewise INT64-only. Both now fall through to the physical type, which is lossless. Twelve honest bytes beat a wrong date. THE WRITE DEFECT. `SignedOrderMatchesLogical` decides whether the deprecated `Statistics.min`/`max` may be emitted, and answered `true` for every `TimestampType`. Its real precondition is narrower than "this Arrow type is signed": it is that `StatisticsCollector` compared the values with a TYPED comparator, which it does only for BOOLEAN/INT32/INT64/FLOAT/DOUBLE. Every FIXED_LEN_BYTE_ARRAY column goes through `SequenceCompareTo` -- unsigned lexicographic. A wrong bound in the footer is a wrong prune, not a cosmetic defect, so the physical type is now part of the answer. This one is latent until an Arrow `TimestampType` can map to FLBA, which is exactly what the FLBA(12) writer will do. There is therefore no end-to-end write that reaches it yet, and a unit test is the only thing standing between the fix and a silent regression -- hence `SignedOrderMatchesLogical` becoming internal. NOT FIXED HERE, deliberately: the same class of mismatch exists for other annotations (STRING on FLBA, DATE on INT64, the fixed-width INT variants). Those need a physical-type compatibility table and a decision about how lenient to be with files that currently "work", which is a bigger change with real regression risk. parquet-testing#122 adds a fixture for that class; it deserves its own PR. Verified: reverting the four guards fails 8 of the 16 new tests. Full Parquet suite 1020/1020 on net10.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(parquet): read back the FLBA columns we write as DELTA_BYTE_ARRAY This library wrote files it could not itself read. Any FIXED_LEN_BYTE_ARRAY column written with ByteArrayEncoding.DeltaByteArray and V2 pages came back as a NullReferenceException -- DECIMAL above precision 18, UUID, FLOAT16 and plain fixed binary alike. DELTA_BYTE_ARRAY is legal for both BYTE_ARRAY and FIXED_LEN_BYTE_ARRAY, and EncodingStrategyResolver emits it for both. But DeltaByteArrayDecoder finished by calling ColumnBuildState.AddByteArrayValues, which writes through the data/offsets buffer pair -- and the state allocates that pair only for BYTE_ARRAY columns. A fixed-width column arrives with both buffers null and dies dereferencing them. No test covered the combination in either direction, so nothing caught it. The reconstruction was already producing exactly the right bytes: when every value is the same width, the output is the packed layout the fixed-width buffer wants and the offsets are redundant. So the fix is to copy it straight into ReserveFixedBytes and skip the byte-array bookkeeping entirely. The width now has to reach the decoder, because a fixed-width column's value size is not recoverable from the encoded page -- prefix and suffix lengths are per-value and a malformed file may disagree with the schema. That is also why the width is checked per value rather than trusted: the bulk copy would otherwise shift every later value silently, which is a worse failure than the crash it replaces. Found while checking whether DELTA_BYTE_ARRAY was usable for the FLBA(12) extended-precision timestamp carrier. It is now, but this is a pre-existing bug on its own and predates that work. Verified: reverting the fixed-width branch fails 7 of the 9 new tests. Parquet suite 1029/1029 on net10.0 and 1023/1023 on net472. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(parquet): stop truncating sub-millisecond timestamp statistics bounds A row-group max bound of 1500 microseconds decoded as 0 milliseconds. A predicate of `t > 0.5ms` then compares against that bound, concludes the row group cannot match, and prunes rows that genuinely do. Silent data loss, not a rounding blemish. ParquetStatisticsAccessor converted every timestamp bound through DateTimeOffset.FromUnixTimeMilliseconds, so MICROS and NANOS columns lost everything below a millisecond -- and lost it by truncating TOWARD ZERO, which moves a positive max down and a negative min up. Both directions narrow the range the file claims, which is the unsafe direction. Bounds now go through TICKS. A DateTimeOffset holds 100 ns, so MILLIS and MICROS are exact and NANOS is the only unit that has to round at all. Where rounding is unavoidable the bound moves OUTWARD -- max up, min down -- so the advertised range can only ever be wider than the data, never narrower. TIME(NANOS) had the same truncation and is fixed with it. A bound outside DateTimeOffset's range is now dropped rather than clamped. A clamped bound is indistinguishable from a real endpoint and would prune on a value the file never contained; no bound at all just means no pruning. The two ends are independent, so a representable min still survives a max that is not. The invariant is stated directly as a test: whatever rounding happens, every value in the column still falls inside the range the footer advertises. Found while adding statistics support for the FLBA(12) extended-precision timestamp carrier -- the same decode path, and the same mistake was about to be repeated there. This is a pre-existing bug and is fixed on its own. Verified: restoring the millisecond conversion fails 7 of the 9 new tests. Parquet suite 1038/1038 on net10.0 and 1032/1032 on net472. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(parquet): reject malformed DELTA_BYTE_ARRAY instead of reconstructing it From Copilot's review of #215, and correct. DELTA_BYTE_ARRAY builds each value from the first prefix_length bytes of the PREVIOUS value plus a suffix. Nothing checked that the prefix actually fit inside the previous value. It does not read out of bounds -- the output buffer is sized from the same lengths -- so it read forward into the zero-filled region reserved for the value being reconstructed and produced a value that is neither what was encoded nor an error. A nonzero prefix on the FIRST value is the same bug at index 0, where there is no previous value at all. Both cases decoded silently. Verifying the report turned up three more in the same family: a negative prefix also decoded silently, while a negative suffix and a suffix running past the end of the page threw ArgumentException and ArgumentOutOfRangeException -- a malformed file reported as an internal argument error rather than as a malformed file. One validation pass covers all five. The total is also accumulated as long now. Prefixes let the described output grow faster than the page does, so a malformed page can claim more bytes than an int can hold. The boundary case is explicitly tested: a prefix exactly the length of the previous value is LEGAL -- it is what an encoder emits for a repeated value -- and must not be caught by the check. The payloads are hand-built from two DELTA_BINARY_PACKED blocks, because no encoder here can produce them. Parquet suite 966/966 on net10.0 and 960/960 on net472. (The cloud-emulator tests in this assembly are excluded from those counts: fake-gcs-server is returning stale content hashes locally after many repeated runs this session. They fail 14-15 at random with and without this change, and CI is green on the branch.) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
CurtHagenlocher
added a commit
to clast-project/engineered-wood
that referenced
this pull request
Sep 5, 2026
…12), behind EWPARQUET0004 (#217) * feat(parquet): decode the 96-bit extended-precision timestamp carrier Groundwork for apache/parquet-format#601, which lets TIMESTAMP annotate FIXED_LEN_BYTE_ARRAY(12): a signed two's-complement LITTLE-ENDIAN count of the declared TimeUnit since the Unix epoch. Ninety-six bits covers the whole ANSI SQL TIMESTAMP(9) range; INT64 nanoseconds stops at 1677-09-21 and 2262-04-11. This is the value layer only -- no schema mapping and no reader or writer wiring, so nothing observable changes yet. WHY THE DEPENDENCY IS Clast.DatabaseDecimal AND WHY IT IS NOT Decimal128. The carrier needs >64-bit integer math and netstandard2.0 has no Int128. That package ships a PUBLIC System.Int128/UInt128 polyfill -- undocumented in its description, but there, and one reference covers every TFM here. Decimal128 was the obvious guess and is the wrong type: this is an integer count of units, its scale would be pinned at 0 forever, and Decimal128's CompareTo is scale-aware decimal comparison rather than the byte order the spec defines. The polyfill is PARTIAL. No Parse, no TryFormat, no generic-math interfaces, and several conversions that are implicit on the BCL type are not -- `Int128 | ulong` does not compile, so every widening here is written out. The tests carry their own ParseInt128 for the same reason. THE COMPARATOR DELIBERATELY DOES NOT USE Int128. It runs once per value while collecting statistics, so it reads the high word signed and the low word unsigned straight out of the bytes -- which is also the shape parquet-java landed after review. TheByteComparatorAgreesWithInt128 is what keeps that shortcut honest. CONFORMANCE, NOT SELF-CONSISTENCY. All eighteen encodings in the tests (six timestamps x three units) were confirmed to appear verbatim in flba12_timestamp.parquet, the fixture proposed in apache/parquet-testing#123 -- including the two nanosecond values that need more than 64 bits, one in each direction. So the byte layout is pinned against the reference file rather than against our own encoder. The tests run on net472 as well, which is the leg where the polyfill actually executes rather than the BCL type. Rescaling floors rather than truncates, for the reason the INT96 path already floors: truncation toward zero would make a pre-epoch value round the opposite way from a post-epoch one and stop being monotonic. THE BYTE ORDER IS NOT SETTLED. The proposal, parquet-java#3680 and the fixture are all little-endian, but a co-author argued for big-endian on the spec PR and the approving reviewer said the choice was still open. Nothing on the wire distinguishes the two, so a flip makes already-written files silently wrong-valued rather than unreadable. Every entry point goes through one file so that a flip is a one-file change, and the experimental gate (EWPARQUET0004, still to come) is what carries the risk. Parquet suite 1073/1073 on net10.0 and 1067/1067 on net472; solution builds clean including the net10.0 AOT/trim gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(parquet): read TIMESTAMP on FIXED_LEN_BYTE_ARRAY(12) The read half of apache/parquet-format#600, behind EWPARQUET0004. ExtendedTimestampOutputKind is a SIBLING of Int96OutputKind, not the same enum, and the reason is not stylistic. INT96 carries no logical annotation, so its unit is genuinely the reader's choice and TimestampMicroseconds/TimestampNanoseconds are both meaningful. This carrier declares MILLIS, MICROS or NANOS in the file -- reading at another unit is a rescale, not an output kind. The two also want opposite defaults, since INT96's default is the one that never throws and this one's is not. What they DO share is the narrowing machinery: both are twelve opaque bytes in the value buffer that become eight in place at Build time, so the hook and the idempotence flag are now shared. THE DEFAULT REFUSES SOME LEGAL FILES, BY DESIGN. Arrow timestamps are int64, so `Timestamp` mode cannot represent year 9999 in nanoseconds -- which is not a corrupt file, it is the case the carrier exists for. It reports the row, the value and a remedy rather than wrapping into a plausible-looking date. TimestampMicroseconds spans +/-292,000 years and always produces an answer; FixedSizeBinary declines to interpret. That has a consequence worth naming: any corpus-wide sweep now needs TimestampMicroseconds, because the upstream conformance fixture is unreadable under the default. ReadRowGroupTests' sweep is updated accordingly -- its question is "can every file be read at all", and repeating a refusal that ExtendedTimestampReadTests already covers would only stop it reaching the rest of the file. The same will apply to the compatibility harness and the parquity bridge when they meet such a file. The declared unit is carried on ColumnBuildState because narrowing happens at Build time, where the column descriptor is long out of scope -- and the target Arrow unit alone does not say what to rescale FROM. TESTED AGAINST THE REFERENCE FILE, not against ourselves: flba12_timestamp.parquet from apache/parquet-testing#123, three columns over six timestamps, expectations taken from the fixture's own documented table. The raw-bytes test rebuilds the encoding from those epoch seconds, so it compares the file to the spec rather than to our decoder. One test guards the fixture's premise -- that its two extreme rows really do exceed int64 -- so the refusal above cannot go quiet if the file is ever regenerated. All of it no-ops until #123 merges and the submodule moves; verified locally against the proposed file, where corrupting one expected value fails 8 of the 10. TimestampCarrierGateTests' fall-through assertion for FLBA(12) is replaced rather than deleted: that width now decodes, and the fall-through it was pinning is now pinned at every OTHER width, which is where it still holds. Parquet suite 1096/1096 on net10.0 and 1090/1090 on net472; solution builds clean including the net10.0 AOT/trim gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(parquet)!: default extended timestamps to microseconds, which cannot fail The previous default kept the file's declared unit and REFUSED any value int64 could not hold. That is a defensible trade in isolation and a bad default in practice, and the corpus sweep proved it: adding the upstream conformance fixture to parquet-testing broke ReadRowGroupTests outright, because a plain ParquetFileReader could not read a valid, spec-conforming file. The same would have hit the compatibility harness, the parquity bridge, and anyone calling the reader with no options. So the default is now TimestampMicroseconds, matching Int96OutputKind's default and for the same stated reason: reading a valid file should not require knowing in advance what is in it. Microseconds span +/-292,000 years, so every date the carrier exists to hold survives. The cost is the last three digits of a NANOS column, and Timestamp mode is still there for callers who would rather be told than lose them. The sweep's TimestampMicroseconds workaround is reverted with the default that made it necessary -- it reads the fixture on stock options now, which is the property worth having. CORRECTING MYSELF: the docs said TimestampMicroseconds "never reports a range error". Not true. The carrier holds +/-2^95 units, which in microseconds is far past int64, so an extreme value still overflows and is reported. Nothing representing a date can reach it, but the claim was wrong and is now stated accurately. TimestampMicroseconds takes the 0 slot so `default(ExtendedTimestampOutputKind)` is the default behaviour rather than the strict one -- again as Int96OutputKind does. Breaking against the previous commit only; nothing has shipped. The range message no longer offers TimestampMicroseconds as an escape when it IS microseconds that overflowed, and otherwise says which mode the caller is in rather than only what to switch to. Parquet suite 1099/1099 on net10.0 and 1093/1093 on net472; solution builds clean including the net10.0 AOT/trim gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(parquet): statistics for the extended-precision timestamp carrier Phase 3: min/max in both directions for TIMESTAMP on FIXED_LEN_BYTE_ARRAY(12). THE COMPARATOR. This is the one FLBA column whose bytes are not ordered lexicographically. It is little-endian two's complement, so the most significant byte is LAST and -1 encodes as all-0xFF -- which SequenceCompareTo ranks above every positive value. DECIMAL sidesteps this by being rewritten to big-endian before statistics run; this carrier cannot, because little-endian is what the spec puts on the wire. StatisticsCollector therefore takes a comparator switch for FLBA, and the writer sets it from (Arrow TimestampType, FLBA) -- a pair an Arrow timestamp reaches by no other route, so the parquet logical type is not needed at that point. TheLexicographicComparatorReallyWouldDisagree pins that the default comparator is genuinely wrong here, so the tests above it cannot quietly become tautologies. THE BOUNDS DECODE via BigInteger's byte[] constructor, which reads little-endian two's complement and takes the sign from the top bit of the last byte -- exactly this layout, and exactly why DECIMAL next to it has to reverse first. Verified against the upstream fixture's own footer: apache/parquet-testing#123 carries min = year 0001 and max = year 9999 on all three columns, both far outside int64 nanoseconds, so a reader that could only narrow to int64 would have no bounds to offer at all. A second test reads the column back and checks the footer is not lying about it, which is the property that makes a bound safe to prune on. The write half is still latent -- nothing emits this carrier until phase 4 -- so the collector is exercised directly rather than through a round trip. Parquet suite 1119/1119 on net10.0 and 1113/1113 on net472; solution builds clean including the net10.0 AOT/trim gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(parquet): write TIMESTAMP on FIXED_LEN_BYTE_ARRAY(12) Phase 4, opted into per column with ParquetWriteOptions.ExtendedTimestampColumns. THE PROMOTION IS NEVER AUTOMATIC AND CANNOT BE. An Arrow timestamp is int64, so any value Arrow can hold already fits INT64 with room to spare -- nothing this library can be handed NEEDS the wider carrier. The option exists to produce files in that shape, for interop fixtures and for readers being tested against the proposal. It follows that we cannot write the far-past and far-future NANOSECOND values that motivate the carrier at all: they cannot be expressed in Arrow to begin with. The MILLIS and MICROS columns of the upstream fixture are fully reproducible, and ReproducesTheFixtureEncodingExactly checks all six values of each against the byte sequences confirmed to appear verbatim in that file. converted_type is OMITTED for this carrier. TIMESTAMP_MILLIS and TIMESTAMP_MICROS are defined for INT64 only, so a reader that understands converted types but not the new logical-type carrier would decode twelve bytes as eight. This is not in the spec PR's text -- parquet-java found it in review -- and it has its own test. NESTED PATHS ARE REFUSED RATHER THAN IGNORED. The schema is built by ArrowToSchemaConverter while the physical type the data is written with is decided by NestedLevelWriter, which does not see these options. Honouring a nested request would put FIXED_LEN_BYTE_ARRAY(12) in the footer over pages holding INT64: a well-formed file that is wrong. A column named but not a timestamp is refused for the same reason -- the caller asked for something and would otherwise silently get something else. BOTH WRITERS, ONE ENCODER. The encoder lives on ExtendedTimestamp because the buffered writer is an independent implementation that has drifted from the streaming one before, and a carrier encoded two ways would drift SILENTLY -- both files would be well-formed. BothWritersProduceTheSameBytes pins that. The buffered writer encodes at accumulation time, because its encoders dispatch on the Arrow type and so have to see the carrier rather than a timestamp. That uncovered a double-encode: its dictionary-fallback path reconstructs the column and hands it to ColumnChunkWriter, which would encode the already-twelve-byte values a second time and read them back as int64. Caught by the round trip through both writers; the encode step now runs only while the values are still timestamps. Statistics key off the option and the path rather than the Arrow type, because the type is what the encode step changes. Parquet suite 1130/1130 on net10.0 and 1124/1124 on net472; solution builds clean including the net10.0 AOT/trim gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(parquet): let temporal predicates probe bloom filters Phase 5. A predicate on a DATE, TIME or TIMESTAMP column could never consult a bloom filter, so writing one on such a column bought nothing. The bloom coercion dispatched on the PHYSICAL type alone, and the statistics layer hands temporal literals over as DateOnly / TimeOnly / DateTimeOffset -- none of which any physical arm accepted, so every one fell through to null and the filter went unread. Not a correctness bug (declining to probe only costs a pruning opportunity) but the opportunity was the entire point of writing the filter. Temporal literals are now decided by the LOGICAL type, before the physical dispatch. That covers the extended-precision carrier as well: the filter holds the hash of the bytes as they sit in the file, so a timestamp literal against a promoted column becomes the same twelve little-endian bytes rather than an int64. Ordinary INT64 timestamp columns get it too -- supporting the experimental carrier and not the everyday case would have been a strange place to stop. EXACTNESS IS THE RULE. A literal is worth probing with only if it converts to the column's unit with no remainder. 1500.5 ms is not a MILLIS value, and rounding it would probe for something the caller never asked about; declining means the row group is read, which is always safe. A tick is 100 ns, so NANOS never has to decline and MILLIS/MICROS sometimes do. THE TESTS WERE WRONG FIRST, AND PASSED. Every "this gets pruned" case used a literal outside the column's min/max -- which STATISTICS prune, with or without a bloom filter, so all of them passed with the new coercion disabled. They now probe a GAP: a value inside min/max and absent from the column, which is the only thing a bloom filter can rule out that statistics cannot. Each such test carries its own control that reads the same file with FilterUseBloomFilters off and asserts the row group survives. Disabling the coercion now fails 3 of the 7. The write side needed nothing: the filter is built after the carrier encoding, so it already hashed the twelve bytes that reach the file. Parquet suite 1137/1137 on net10.0 and 1131/1131 on net472; solution builds clean including the net10.0 AOT/trim gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(parquet): record the extended-timestamp decisions, and the diagnostics Phase 7. doc/parquet-extended-precision-timestamps.md, following the parquet-fsst.md shape: what the carrier is, and then the part a reader of the code cannot recover -- which decisions are ours rather than the spec's. The headline is that the BYTE ORDER IS NOT SETTLED. The spec text, parquet-java and the fixture are all little-endian, but a proposal co-author argued for big-endian on the spec PR and the approving reviewer left the choice explicitly open. Nothing on the wire distinguishes the two, so a flip makes already-written files silently wrong-valued rather than unreadable. The doc carries a four-item list of exactly what would change, since that is the question anyone will have. Also recorded: that Arrow has no type for this and no plan for one, so the read mapping is our choice and not a standard; that the default was `Timestamp` for one commit and the corpus sweep is what changed it; that converted_type suppression is parquet-java's finding and not in the spec PR's text; that we deliberately did not invent an Arrow extension name; and that the promotion can never be automatic -- with the consequence that this library cannot write the very values the carrier exists for, because they cannot be expressed in Arrow. Validation gets its own section INCLUDING ITS LIMITS: there is no external oracle, which is weaker than ALP and FSST had, and it compounds the endianness risk. What we do have is stated precisely enough to be checked. README gains a diagnostics table, which BACK-FILLS EWPARQUET0002 -- it gated the one option here that produces files no other implementation can read, and it was documented nowhere but its own XML comment. known-issues.md gains the three residual limits (top-level only, no Arrow extension type, cannot write out-of-int64 values), plus a note on the Column Index entry: if page indexes are ever added, these bounds must never be truncated, because truncation assumes lexicographic order and this carrier is little-endian signed. parquet-java had to special-case BinaryTruncator for exactly that. Every relative link checked to resolve. Parquet suite 1137/1137 on net10.0 and 1131/1131 on net472; solution builds clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(parquet): honour slice offsets and stop nanoseconds wrapping Copilot's review of #217: three comments, all three correct. THE SLICE BUG WAS THE WORST. ExtendedTimestamp.EncodeColumn sliced the VALUE buffer by Data.Offset and then handed back the caller's validity bitmap alongside offset 0 -- so bit i was read where bit (offset + i) was meant, and every null moved. Measured on a three-row slice of a five-row column: expected 30, null, 50; got null, 40, 50. Both the values and the nulls, and a perfectly well-formed file either way. Reachable through BufferedParquetWriter, which takes sliced arrays as they come: it tracks Data.Offset rather than compacting, unlike ParquetFileWriter, whose CompactSlicedColumns runs first and hid the problem on that path. The bitmap is now rebuilt to match the compacted values -- and NOT skipped when the null count is zero, because a bitmap can be fully set across the slice while holding zeros outside it, which read at offset 0 invents nulls. NANOSECONDS WRAPPED AT THE END OF THE SQL RANGE. Converting ticks to nanoseconds as `ticks * 100` in 64 bits overflows: year 9999 is 2.53e20 nanoseconds and a long holds 9.22e18. It wrapped to -4852116231933722724. As a bloom-filter probe that means asking whether some OTHER timestamp is present, and being told "absent" is what prunes a row group -- so a file whose column genuinely holds that value could lose it. Not reachable through our own writer, since Arrow cannot express such a value in the first place, but perfectly reachable in a file parquet-java wrote. The conversion now happens in Int128 and moves to ExtendedTimestamp, where the rest of the unit arithmetic already lives. An INT64 column range-checks the result and declines rather than probing: a value that type cannot hold is not in it. The carrier does the same against +/-2^95. TIME(NANOS) keeps its 64-bit multiply and now says why -- a time of day is at most 8.64e13 nanoseconds. THE DOC WAS STALE. MakeExtendedTimestampArrowType still said the default keeps the file's declared unit. That stopped being true when the default became TimestampMicroseconds, and I missed the comment. Each fix verified by reverting it: the two correctness fixes fail 2 of the 57 carrier tests when undone. Parquet suite 1113 passed / 37 skipped on net10.0, 1107 / 37 on net472; solution builds clean including the net10.0 AOT/trim gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: CurtHagenlocher <904803+CurtHagenlocher@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
This PR implements the changes described in the proposal document to add support for extended precision nanosecond timestamps that cover the full ANSI SQL timestamp range (years 0000-9999).
What changes are included in this PR
Spec changes to allow the
TimestampTypelogical type to annotate theFIXED_LEN_BYTE_ARRAYphysical type withtype_length = 12.Do these changes have PoC implementations?
The parquet-java change is in progress.
Closes #600