[brkyvz]

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:

uri 		 		STRING	
offset				INT64      
size	 	 		INT64	
content_type		 STRING
checksum		 	STRING
inline 		 		BINARY

Are these changes tested?

Yes, unit tested

Are there any user-facing changes?

Introduces a new LogicalTypeAnnotation called FILE

[alkis]

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 fileType).

The main piece is the writer-side API you'd need anyway: FileValueWriter.write(payload) returns either inline bytes or an offset/size pair based on a configurable threshold (ParquetProperties.withFileSelfReferenceThreshold), so object models don't choose between the two forms themselves. It has to write the payload eagerly, mid-record — offset/size are ordinary column values, so once one reaches a ColumnWriter it's encoded into a buffered page and a placeholder can't be patched up later.

Three things worth flagging regardless of whether you take the PR:

1. The AAD uses a per-chunk counter, but the spec defines field 6 as the file offset. The counter isn't recoverable from the file — nothing stores it, so a reader has to walk the preceding values to rebuild it, which defeats exactly the property the offset-keyed AAD is there for ("available to a reader without counting preceding values... may therefore resolve a self-reference without decoding the pages it skips"). Switching to offset also drops the parameter from five signatures, since the value already carries it.

2. decompressUnknownSize doesn't work for any codec, including SNAPPY. The drain loop ends on read() == -1, but NonBlockedDecompressorStream throws IOException("Corrupt file: Zero bytes read during decompression.") when its block is consumed rather than returning -1. It was never caught because CI dies at compile. Replaced with grow-and-retry into a dynamically sized buffer, which the spec explicitly allows; that covers LZ4_RAW too.

3. parquet-hadoop has two test compile errors that block the module independent of anything I changed (they fail on be266d31): TestParquetMetadataConverter calls assertEquals/assertTrue with no JUnit import, and TestSelfReferenceFileWrite is missing the ParquetReadOptions import. Fixing those is what let me actually run things — currently 679/679 in parquet-column and 761/762 in parquet-hadoop.

One thing I deliberately did not change: I'd tightened schema validation to require inline whenever offset is declared, on the reasoning that uri is optional per value so a uri+offset+size schema can still emit a self-reference with no inline column chunk to inherit from. But your tests assert that schema is valid, so I reverted it — your call which way it should go.

Also worth noting for sequencing: this can't go green until a parquet-format release carries FileType (2.13.0's LogicalType union stops at 18 — that's the current CI failure), and #603 is still open, so the AAD detail could still move. To build locally I pinned parquet.format.version to an artifact holding parquet-format master's parquet.thrift.

Happy to fold any of it in differently, or split it up, if that's easier for you.

[emkornfield]

I think the read side of self-reference vs inline needs work here, it seems that we aren't actually doing a full round trip but relying on in-memory structures to retrieve the bytes.

[divjotarora]

It seems Types.java is the wrong place for some of the FILE validity checks as it only sees the schema, not the actual values. We probably need specialized checks in the writer and reader paths to check individual rows. I'm not sure where the best place to put this would be. Maybe check how VARIANT does this? At a high level, Codex claims:

• VariantConverters validates the group’s schema fields.
• Variant validates actual encoded metadata values.
• VariantValueWriter checks value/schema compatibility while writing.

And that you could do something similar:

• FileValue representing one assembled FILE value.
• FileConverters collecting child columns and validating in GroupConverter.end().
• FileValueWriter validating before emitting fields.
• A shared validator for offset/uri/size, non-negativity, and resolvability.

[emkornfield]

CI failure: 'TestParquetMetadataConverter.testFileLogicalTypeIsIgnoredRatherThanFailing`

[brkyvz]

@divjotarora addressed your comments. Added reader/writer interfaces

[divjotarora]

Hey @brkyvz, how should FileConverters behave with projected FILE groups? For example, suppose a valid stored value has uri and content_type, but the reader projects only content_type. The converter never sees uri, builds a value without a locator, fails validation, and returns null. It therefore cannot distinguish "URI was null in the file" from "URI was not projected." Is the intended contract that FILE projections must include enough fields to form a valid locator? If so, that should probably be documented/enforced, otherwise the converter needs to support partial values.

[brkyvz]

Thanks @divjotarora great point! Added checks in parquet-column/src/main/java/org/apache/parquet/io/ColumnIOFactory.java that avoid partial projections of FileType. It should be treated atomically

[gunnarmorling]

Hey, I've sent PR apache/parquet-testing#128 for adding FILE fixtures to parquet-testing, it might be interesting for testing here?

[Fokko]

Thanks for working on this @brkyvz 👍 Left some comments, mostly around validation of the fields.