fix(parquet): three pre-existing defects — unreadable FLBA files, truncated bounds, an ungated annotation - #215
Merged
Conversation
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>
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>
…unds 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>
There was a problem hiding this comment.
Pull request overview
This PR addresses three correctness issues in the Parquet reader/writer and statistics handling: (1) gating TIMESTAMP logical/converted types on INT64 carriers to avoid silent mis-decoding, (2) fixing DELTA_BYTE_ARRAY decoding for FIXED_LEN_BYTE_ARRAY so written files can be read back, and (3) preserving sub-millisecond precision in decoded statistics bounds to avoid unsafe row-group pruning.
Changes:
- Gate Arrow TIMESTAMP type mapping on physical INT64 for both LogicalType and ConvertedType paths, and refine deprecated min/max emission gating based on physical type.
- Fix DELTA_BYTE_ARRAY decoding for FIXED_LEN_BYTE_ARRAY by writing into the fixed-width destination buffer and validating per-value width.
- Decode timestamp/time statistics bounds via ticks with outward rounding (when needed) and drop non-representable bounds; add targeted regression tests for all three defect classes.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/EngineeredWood.Parquet.Tests/Parquet/Data/TimestampCarrierGateTests.cs | Adds unit tests pinning TIMESTAMP carrier gating and deprecated-bound emission behavior. |
| test/EngineeredWood.Parquet.Tests/Parquet/Data/TimestampBoundPrecisionTests.cs | Adds end-to-end tests ensuring stats bounds preserve sub-ms precision and round outward safely. |
| test/EngineeredWood.Parquet.Tests/Parquet/Data/FlbaDeltaByteArrayTests.cs | Adds round-trip and malformed-input tests for FLBA + DELTA_BYTE_ARRAY (V2 pages). |
| src/EngineeredWood.Parquet/Parquet/ParquetStatisticsAccessor.cs | Changes timestamp/time bound decoding to tick-based conversion with outward rounding and range dropping. |
| src/EngineeredWood.Parquet/Parquet/Data/DeltaByteArrayDecoder.cs | Adds FIXED_LEN_BYTE_ARRAY support and per-value width validation in DELTA_BYTE_ARRAY decode. |
| src/EngineeredWood.Parquet/Parquet/Data/ColumnChunkWriter.cs | Refines deprecated min/max emission gating by incorporating physical type into signed-order checks. |
| src/EngineeredWood.Parquet/Parquet/Data/ColumnChunkReader.cs | Passes type_length into DELTA_BYTE_ARRAY decoding to support FLBA destinations safely. |
| src/EngineeredWood.Parquet/Parquet/Data/ArrowSchemaConverter.cs | Gates TIMESTAMP mapping on INT64 for both logical and converted types to avoid silent buffer reinterpretation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ting 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>
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
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.
Three pre-existing Parquet defects, found while implementing the extended-precision timestamp
carrier (parquet-format#600) but caused by
none of it. Split out so they are not gated on review of an unratified format whose byte order is
still being decided upstream.
Two of the three are in the "silently wrong" class rather than the "throws" class.
1.
TIMESTAMPwas mapped to an Arrow timestamp without checking the physical typeArrowSchemaConverter.FromLogicalTypehanded back an ArrowTimestampTypefor any TIMESTAMPannotation — unlike
MakeDecimalType, which has always switched oncolumn.PhysicalType. The readpath maps
Int64Type or TimestampType or Time64Typeonto alongvalue buffer, so a TIMESTAMPannotation on a 12-byte column was reinterpreted eight bytes at a time and produced
plausible-looking wrong dates. The same hole existed on the converted-type path, where
TIMESTAMP_MILLIS/TIMESTAMP_MICROSare likewise INT64-only.Both now fall through to the physical type, which is lossless.
The commit also narrows
SignedOrderMatchesLogical, which gates the deprecatedStatistics.min/max. That half is latent — it only matters once an ArrowTimestampTypecanmap to FIXED_LEN_BYTE_ARRAY, which nothing here does — so it is covered by a unit test rather than
a round trip. It is included because it is the same assumption, in the same shape, one function
away.
2. FIXED_LEN_BYTE_ARRAY written as DELTA_BYTE_ARRAY could not be read back
This library wrote files it could not itself read. Any FLBA column written with
ByteArrayEncoding.DeltaByteArrayand V2 pages came back as aNullReferenceException—DECIMALabove precision 18,UUID,FLOAT16and plain fixed binary alike.DeltaByteArrayDecoderfinished by callingColumnBuildState.AddByteArrayValues, which writesthrough the data/offsets buffer pair that the state allocates only for BYTE_ARRAY columns. A
fixed-width column arrived with both buffers null. No test covered the combination in either
direction.
When every value is the same width the reconstruction is already the packed layout the fixed-width
buffer wants, so the fix copies it straight into
ReserveFixedBytes. The width is now checked pervalue rather than trusted: the bulk copy would otherwise shift every later value silently, which is
worse than the crash it replaces.
3. Sub-millisecond statistics bounds were truncated
A row-group max bound of 1500 µs decoded as 0 ms. A predicate of
t > 0.5msthen comparesagainst that bound, concludes the row group cannot match, and prunes rows that genuinely do.
ParquetStatisticsAccessorconverted every timestamp bound throughDateTimeOffset.FromUnixTimeMilliseconds, so MICROS and NANOS columns lost everything below amillisecond — and lost it by truncating toward zero, which moves a positive max down and a
negative min up. Both narrow the range the file claims, which is the unsafe direction.
Bounds now go through ticks. A
DateTimeOffsetholds 100 ns, so MILLIS and MICROS are exact andNANOS is the only unit that rounds at all; where it must, the bound moves outward.
TIME(NANOS)had the same truncation and is fixed with it. A bound outsideDateTimeOffset'srange is dropped rather than clamped, since a clamped bound is indistinguishable from a real
endpoint.
The invariant is a test in its own right: whatever rounding happens, every value in the column
still falls inside the range the footer advertises.
Verification
Each fix was checked by reverting it and re-running — a test that passes either way is not
testing the fix:
Full Parquet suite at the tip: 1038/1038 on net10.0, 1032/1032 on net472. Solution builds
clean including the net10.0 AOT/trim gate.
Deliberately not fixed here
The same physical-vs-logical mismatch exists for other annotations — STRING on FLBA, DATE on INT64,
the fixed-width INT variants. Those need a compatibility table and a decision about how lenient to
be with files that currently "work", which carries real regression risk.
parquet-testing#122 adds a fixture for that
class; it deserves its own change.
🤖 Generated with Claude Code