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>
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
4 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