alkis commented on PR #3608:
URL: https://github.com/apache/parquet-java/pull/3608#issuecomment-5195437021
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/parquet-java#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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
