zhuqi-lucas commented on PR #11168: URL: https://github.com/apache/arrow-rs/pull/11168#issuecomment-5884976467
Thanks @etseidl — applied all of them, the docs read much better trimmed. One heads-up: applying the suggestions moved `read_page_header_len_from_bytes` out of the `SerializedPageReader` impl and updated only one of its two call sites, so the branch went red for a moment. Fixed in d2b129435. You are right about the corrupt dictionary, and it is sharper than "may not trigger an error" — it definitely will not, for any chunk that decodes nothing. Today the page is decompressed the moment the skip path reaches it, so corruption fails even when the chunk goes on to decode zero values; with the deferral those bytes are never touched. A chunk that does decode something still hits the error, just later. No wrong data is returned either way, but corruption in a never-read dictionary now goes unreported. It is also the mechanism behind two of the tests: `corrupt_dictionary_body` zero-fills the dictionary page body, and the same corrupted file makes `skipping_a_whole_chunk_never_decodes_the_dictionary` pass while `a_partial_skip_pays_the_deferred_dictionary` fails — "does this error" is the probe for "was it decompressed". I have written the behaviour into the user-facing changes section. -- 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]
