Introduce implementation for FileType - #3608
Conversation
| } else if (!LogicalTypeAnnotation.FileLogicalTypeAnnotation.OPTIONAL_FIELD_NAMES.contains(fieldName)) { | ||
| throw new IllegalArgumentException( | ||
| "FILE type group '" + name + "' contains unrecognized field '" + fieldName | ||
| + "'. Valid fields are: path, size, offset, etag"); |
There was a problem hiding this comment.
nit: stringify FileLogicalTypeAnnotation.OPTIONAL_FIELD_NAMES and use it in the error to avoid drift if we add new fields?
There was a problem hiding this comment.
We should add tests for validity of size/offset fields, both their values (>= 0) and valid/invalid combinations
There was a problem hiding this comment.
where can we add value checks during reads and writes? Can you provide any code references please?
|
I had a look at this against the revised self-reference semantics in apache/parquet-format#603 and ended up writing some of it, so rather than leave it as review comments I opened it as a PR against this branch: brkyvz#1 (delta is just two commits on top of The main piece is the writer-side API you'd need anyway: Three things worth flagging regardless of whether you take the PR:
One thing I deliberately did not change: I'd tightened schema validation to require Also worth noting for sequencing: this can't go green until a parquet-format release carries Happy to fold any of it in differently, or split it up, if that's easier for you. |
…#1) * Add threshold-driven inline vs self-reference storage for FILE values Writers hand FILE payloads to FileValueWriter, which decides from a configured threshold whether to keep the bytes inline or store them out of line as a self-reference. Both forms describe the same logical bytes, so content_type and checksum are written identically either way and consumers see no difference beyond which fields are set. The payload is written eagerly, while the record is being written and before the row group's column chunks are flushed. This is what makes the offset knowable in time: offset and size are ordinary column values, and once a value reaches a column writer it is encoded into a buffered page and cannot be revised, so a placeholder could not be patched up later. Writing at that point also leaves each column chunk contiguous on disk, which the read path relies on when coalescing chunks into range reads. Along the way, per parquet-format#603: - Key the self-reference AAD on the file offset rather than a synthetic per-chunk counter. The spec defines AAD suffix field 6 as the offset, and the counter was not recoverable from the file: nothing stores it, so a reader had to walk preceding values to rebuild it, defeating the offset-based design that lets a value be resolved on its own. The ordinal parameter is gone from five signatures as a result. - Decompress into a dynamically sized buffer, supporting every codec. The previous stream-drain loop was broken for all codecs, not just unframed ones: NonBlockedDecompressorStream throws rather than returning -1 once its block is consumed. - Enforce the 2 GiB encrypted-module limit on write, directing oversized values to external references. - Require inline whenever offset is declared. uri is optional per value, so a uri+offset+size schema still permits self-references, and one emitted without an inline column chunk has no reference point to inherit compression and encryption from. Tests cover threshold routing, offset-keyed AAD round-trips in both GCM and CTR modes, tamper detection when an offset is altered, and payloads spanning several buffer doublings across SNAPPY, GZIP, ZSTD and LZ4_RAW. Co-authored-by: Isaac * Fix ZSTD resolution, revert schema tightening, fix test imports Verified the change by building and running the suites locally, which turned up three things: ZSTD self-references could not be resolved at all. ZstandardCodec returns null from createDecompressor because it decompresses only through its stream, so driving the Decompressor directly failed with "Could not obtain a decompressor". Such codecs are framed and report end-of-input properly, so drain the stream for them and keep grow-and-retry for the rest. Four tests were failing on this. Reverted requiring `inline` whenever `offset` is declared, back to requiring it only when `uri` is absent. Two existing tests (testFileLogicalTypeExternalRangedReferenceWithoutInline, testFileLogicalTypeOffsetWithSize) assert that a uri+offset+size schema without `inline` is valid, so the stricter rule contradicted the author's documented intent. The concern is real -- `uri` is optional per value, so such a schema can still emit a self-reference with no reference point -- but it belongs on the write path, and the FILE group declaring `uri` is now documented as needing an always-inline threshold. Fixed two pre-existing test compile errors that blocked the module: TestParquetMetadataConverter used assertEquals/assertTrue with no JUnit import (switched to the AssertJ style used throughout that file), and TestSelfReferenceFileWrite was missing the ParquetReadOptions import. Both fail on the base commit independently of these changes. Also applied spotless formatting. Local results: parquet-column 679/679 pass, parquet-hadoop 761/762. The one failure, testEnumEquivalence on Encoding.ALP, is an artifact of the local workaround for FileType being unreleased -- parquet.thrift was substituted from parquet-format master, which defines an ALP encoding the Java enum does not yet have. It is unrelated to these changes and will not occur once a parquet-format release carries FileType. Co-authored-by: Isaac
4 participants
Rationale for this change
Introduces the reference implementation for the new LogicalTypeAnnotation File as discussed in this design document
What changes are included in this PR?
Introduces a new LogicalTypeAnnotation called FILE. It enforces that a group with this type annotation have the following fields by name:
Are these changes tested?
Yes, unit tested
Are there any user-facing changes?
Introduces a new LogicalTypeAnnotation called FILE