discivigour commented on PR #9928: URL: https://github.com/apache/paimon/pull/9928#issuecomment-5724771032
> Verified the Avro-side safety of this against avro-1.11.4 internals (disassembled `DataFileWriter`/`BufferedBinaryEncoder`), since the risk here is a buffered `bufOut` desynchronizing the block buffer. All three append paths are flush-safe with the buffered encoder: > > * `append()` rollback uses `resetBufferTo(bufferInUse)`, which flushes `bufOut` before truncating — so a failed append can't leave partial encoder bytes that survive into the next record. `testBufferedRecordSerializationFailureRollsBack` pins exactly this, including the partial-encoder-buffer case (small payload) vs. flushed case (large payload). > * `appendEncoded()` goes through `bufOut.writeFixed`, and `writeBlock()` flushes `bufOut` before the raw block copy, so `appendAllFrom` can't interleave encoder residue with a copied block. `testRecordsAroundRawBlockCopy` (rows + encoded records + raw block copy, across all four codecs) covers the ordering. > * `setEncoder` is applied at `create()`, before any append — timing is fine. > > Keeping the direct encoder for manifests via `context.isManifest()` is the right call: the buffered encoder copies array-backed `ByteBuffer`s into a temp array on every `appendEncoded`, which is the manifest hot path. And excluding manifests from `FILE_BLOCK_SIZE` (sync interval) while data files get it is consistent with the existing `manifestFormat` behavior. > > `testCloseFlushesBufferedRecords` covering all codecs is appreciated — the "leave both a partial encoder buffer and a partial block, then close" setup is exactly the state that would silently truncate. Thanks for the detailed review and for checking the Avro 1.11.4 internals! I agree that keeping Direct for manifests avoids the extra allocations while retaining Buffered for data-file writes. -- 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]
