[improvement](be) Enable libdeflate decompression for Parquet GZIP and ORC ZLIB on all architectures - #68593
Open
hubgeter wants to merge 4 commits into
Open
[improvement](be) Enable libdeflate decompression for Parquet GZIP and ORC ZLIB on all architectures#68593hubgeter wants to merge 4 commits into
hubgeter wants to merge 4 commits into
Conversation
… all architectures Parquet GZIP pages were decompressed by libdeflate only on x86; AArch64 fell back to zlib inflate. libdeflate is already built for every platform by build-thirdparty.sh and ships in both linux prebuilt packages, so link it unconditionally and use GzipBlockCompressionByLibdeflate everywhere. Add a unit test that checks the Parquet GZIP codec against the zlib gzip codec for block sizes from 1 byte to 8 MB, and that empty input, an undersized output buffer, truncated streams, corrupted header/deflate/CRC bytes and non-gzip input are handled with an error instead of partial data.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
hubgeter
marked this pull request as draft
September 29, 2026 02:06
…on all architectures Pick up apache/doris-thirdparty#417, which removes the x86-only guards around ZlibDecompressionStreamByLibDeflate, so ORC ZLIB streams are decoded with libdeflate on AArch64 as well.
hubgeter
changed the title
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes for two AArch64 regressions, detailed inline: a zero-byte Parquet GZIP dictionary page can be accepted without producing its declared bytes, and ORC ZLIB streams now allocate an avoidable block-sized scratch buffer per stream.
Review checkpoints:
- Goal and tests: The CMake link, Parquet codec selection, and ORC submodule implement the intended all-architecture switch. The new BE and ORC unit cases cover normal and malformed streams, but the BE test misses empty compressed input with a positive expected output size, and the ORC test does not cover per-stream memory. This was a static review; I did not run builds or tests, and the PR's reported test results were not independently verified.
- Scope and reuse: The patch reuses existing codecs and is focused, subject to the two inline corrections. No new configuration, FE/BE variable, persisted format, or rolling-upgrade protocol is added.
- Data correctness and errors: The Parquet case violates the codec's output-size/error contract and lets the legacy reader consume unwritten dictionary bytes. Other observed decompression errors propagate as Status or ParseError. No transaction, visible-version, data-write, or crash-recovery flow changes here.
- Concurrency and lifecycle: The BE libdeflate decompressor is thread-local; ORC owns a decompressor per stream and releases it with the reader. I found no new shared-state race, lock-order issue, or cross-translation-unit initialization dependency. The ORC scratch allocation lasts for the live stream/stripe.
- Parallel paths and compatibility: I traced both Parquet readers, ORC chunk and seek handling, source/prebuilt dependency layout, and Linux/macOS link order. No additional valid-stream or link failure was substantiated. A final glibc/OS artifact audit is unavailable without a build, which this review contract prohibits.
- Performance, coverage, and observability: The ORC buffer cost is the material performance finding; neither added test measures it. No test-result files or observability interfaces change. No additional user review focus was supplied.
Review complete after two rounds of independent coverage and a final changed-file sweep. Both inline issues remain open for the author to address.
| @@ -1663,13 +1653,7 @@ Status get_block_compression_codec(tparquet::CompressionCodec::type parquet_code | |||
| *codec = ZstdBlockCompression::instance(); | |||
| break; | |||
| case tparquet::CompressionCodec::GZIP: | |||
Contributor
There was a problem hiding this comment.
[P1] Reject empty GZIP input when the page expects output. On AArch64 this now selects GzipBlockCompressionByLibdeflate, whose empty-input branch returns OK and sets output->size = 0 even when the caller provided a nonzero destination. A dictionary page with num_values=1, uncompressed_page_size=4, and compressed_page_size=0 reaches the legacy Parquet reader; it checks only Status and then reads the unwritten four-byte allocation as an INT32 dictionary value. The previous ARM zlib path rejected that page, and the new test exercises empty input only with an empty destination. Please reject this size mismatch and test it.
| @@ -1 +1 @@ | |||
| Subproject commit be0f1b73a7aeb78824a03e0dcb692c50a176d513 | |||
| Subproject commit 34a46c9e2de3214eea527f50c45c94264c14cdcd | |||
Contributor
There was a problem hiding this comment.
[P2] Allocate the ORC input scratch only when a chunk spans input buffers. On ARM this submodule update switches ZLIB streams to ZlibDecompressionStreamByLibDeflate, whose BlockDecompressionStream base immediately allocates and zeroes inputDataBuffer(pool, blockSize) for every stream, in addition to the existing output buffer. A nullable direct-string column opens PRESENT, LENGTH, and DATA streams; 100 selected columns at a valid 256 KiB block size therefore add about 75 MiB per active reader before data is read. Doris defaults to an 8 MiB natural input read, so this scratch is often unused. NextDecompress needs it only for split chunks; lazy allocation would preserve the fast path without this wide-scan memory cost.
Contributor
TPC-H: Total hot run time: 27528 ms |
Contributor
TPC-DS: Total hot run time: 153283 ms |
Contributor
ClickBench: Total hot run time: 24.02 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…d size GzipBlockCompressionByLibdeflate returned OK for an empty page even when the page header declared uncompressed bytes, and accepted a page that expanded to fewer bytes than declared. The Parquet readers only check the Status, so a dictionary page with compressed_page_size=0 left its dictionary buffer unwritten and decoded it as values. Page headers give the exact uncompressed size, so require the page to fill the output buffer: accept empty input only for an empty output, and let libdeflate reject short output by not passing actual_out_nbytes_ret.
… input buffer lazily Pick up apache/doris-thirdparty#418, so ORC block decompression streams no longer allocate an input scratch block per stream unless a chunk spans input buffers.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Complete static review of PR #68593 at head 3b7ad3b. One new P2 issue is inline: the updated ORC submodule leaves its standalone ARM test and tool targets without a libdeflate link dependency. The normal Doris BE link includes deflate.
Existing review context: P1 comment 4129170058 is fixed by rejecting empty GZIP input when output bytes are declared and by using libdeflate's exact-output mode; the added unit case covers that input. P2 comment 4129170064 is fixed by ORC commit 5860f947, which grows input scratch only for a split chunk. Neither prior issue still applies, so there is no existing blocking comment ID for this head.
Review checkpoints:
- Goal and proof: Parquet GZIP and ORC ZLIB select libdeflate on ARM, and the BE dependency list includes it. The new BE and ORC unit cases exercise normal, malformed, contiguous, and split input. I did not run builds, tests, benchmarks, or an ELF audit because this review environment prohibits builds; test results stated in the PR were not independently verified.
- Scope and reuse: The change is focused on codec selection, its dependency, targeted tests, and the ORC submodule. It reuses the existing codec and stream interfaces. The ORC target link declaration is the remaining integration gap.
- Concurrency and lifecycle: The BE decompressor is thread-local; each ORC stream owns its decompressor and scratch through the reader's memory pool. No new shared mutable state, lock acquisition, deadlock path, cross-translation-unit initializer dependency, or abnormal release path was found.
- Configuration and compatibility: No configuration item, FE/BE variable, protocol, function symbol contract, or persisted storage format changes. Thirdparty source builds include libdeflate and the BE link includes it; ORC's own executables do not inherit that link. No built artifact was available to verify the glibc 2.17 runtime baseline or ARM binary behavior.
- Parallel paths and conditions: Both legacy and native Parquet readers pass declared sizes through cold and compressed-cache data and dictionary paths, including page-v2 level subtraction, and propagate decompression failures. The empty-input exception is limited to zero expected output. ORC contiguous/split chunks and seek/BackUp paths were reviewed; lazy scratch does not recreate the prior per-stream block allocation.
- Tests and results: The BE unit test covers valid sizes, short/long output, truncation, corruption, and empty input; ORC submodule tests cover malformed chunks and memory-pool allocation for contiguous/split inputs. No end-to-end regression test or result file changed. This is static coverage assessment, not a claim that tests passed here.
- Observability and performance: New Parquet failures include the libdeflate result and input/output sizes; existing ORC ParseError behavior remains. No new metric is required by this decoder switch. The large eager ORC scratch cost is fixed; throughput improvement was not independently measured.
- Data safety and remaining scope: These are read/decompression paths; no transaction, visible-version, delete-bitmap, persistence, data-write, crash-recovery, or cloud write flow changes. Errors propagate instead of allowing a short decoded page to be consumed. No additional user review focus was supplied.
Review completed after two convergence rounds, independent validation of the one new finding, and a final sweep of all four changed paths and live inline threads.
| @@ -1 +1 @@ | |||
| Subproject commit be0f1b73a7aeb78824a03e0dcb692c50a176d513 | |||
Contributor
There was a problem hiding this comment.
[P2] Link libdeflate from ORC's own targets on ARM. This revision makes ZlibDecompressionStreamByLibDeflate unconditional, so liborc.a now references libdeflate_* on ARM. The submodule's orc target still links only protobuf, zlib, snappy, lz4, zstd, and HDFS; orc-test and tools such as orc-contents link through that target without libdeflate. Building those targets on ARM therefore fails with undefined libdeflate symbols. Doris's COMMON_THIRDPARTY link group covers doris_be, not these executables. Please declare libdeflate on the ORC target and update this submodule pointer.
Contributor
TPC-H: Total hot run time: 27507 ms |
Contributor
TPC-H: Total hot run time: 28216 ms |
Contributor
TPC-DS: Total hot run time: 152455 ms |
Contributor
ClickBench: Total hot run time: 24.03 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hubgeter
marked this pull request as ready for review
September 29, 2026 13:00
Contributor
|
Reviewed head One build dependency issue remains, also noted in the existing inline comment: ORC's own target does not declare its libdeflate dependency. The pinned ORC revision now calls Doris's final BE link includes libdeflate through Please declare libdeflate as a transitive dependency of the ORC target, ensure the standalone build can discover its headers/library, and update the submodule pointer. Validate an ORC test/tool executable as well as the Doris BE build on ARM64. This conclusion is based on source and CMake inspection. I did not run an ARM64 build or reproduce the linker failure locally. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Related PR: apache/doris-thirdparty#417, apache/doris-thirdparty#418
Problem Summary:
Parquet GZIP pages and ORC ZLIB streams are decompressed with libdeflate only on x86 (#27542, #27669). On AArch64 both fall back to zlib
inflate, which is several times slower. The x86-only restriction was never needed:build-thirdparty.sh.doris-thirdparty-prebuilt-linux-aarch64package ships an aarch64libdeflate.a, reachable through the usualinstalled/lib -> lib64link.This PR:
deflateunconditionally inbe/cmake/thirdparty.cmake;be/src/util/block_compression.cpp, so ParquetGZIPalways usesGzipBlockCompressionByLibdeflate;contrib/apache-orcto include [improvement] Enable libdeflate for ZLIB decompression on all architectures doris-thirdparty#417, which does the same for ORCCompressionKind_ZLIB(ZlibDecompressionStreamByLibDeflate).GzipBlockCompressionByLibdeflaterequire a page to expand to exactly the declared size.compressed_page_size=0could be decoded from an unwritten buffer.actual_out_nbytes_ret).contrib/apache-orcto include [improvement] Allocate the block decompression input buffer lazily doris-thirdparty#418, which allocates ORC's block decompression input buffer lazily, so libdeflate ZLIB streams no longer hold an extra block per stream.Release note
Use libdeflate to decompress Parquet GZIP and ORC ZLIB data on ARM64.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)