[fix](parquet) Bound Thrift and Bloom filter metadata allocations - #66820
[fix](parquet) Bound Thrift and Bloom filter metadata allocations#66820Gabriel39 wants to merge 4 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the patch usefully bounds Bloom header I/O and hostile Thrift prefixes, but three P1 correctness/resource-budget defects and two mutation-sensitive P2 test gaps remain.
Critical checkpoint conclusions:
- Goal and proof: partial. Contradictory Bloom metadata can still cause false-negative pruning; accepted Bloom filters and decoded Thrift containers can still exceed task admission; the tests do not prove every new guard.
- Scope, clarity, and reuse: the five-file change is focused and readable, and the shared Thrift utility is the right boundary. The same exact-length and tracked-ownership fixes must also cover the parallel format_v2 Bloom path.
- Conditions, arithmetic, and error handling: signed Bloom metadata and file-range arithmetic are otherwise bounded, new I/O statuses are checked, and malformed filters fall back conservatively. The remaining bad conditions are the non-exact declared Bloom length and treating serialized bytes as a decoded-memory budget.
- Concurrency, lifecycle, and static initialization: no new locks, shared state, cycles, or static-order dependencies were introduced. Concurrent scanners amplify the unreserved allocations, and eventual RAII cleanup occurs only after the excessive peak or cache retention.
- Configuration: no configuration was added. The existing
thrift_max_message_sizecaps serialized format_v2 footer bytes, not decoded object storage. - Compatibility: the Thrift 0.16 constructor arguments and binary strict flags preserve wire behavior; zero and
INT32_MAXhandling remain compatible. Bloom metadata withoutbloom_filter_lengthremains supported, while equality for a present field follows the Parquet contract. - Parallel callers and implementations: compact Parquet and binary service/plan/profile callers share the decoded-container issue. Legacy and format_v2 Bloom readers share the exact-length and untracked-copy issues; their non-OK paths remain conservative.
- Performance and memory: the fixed 64-byte header read is a real improvement and the new scalar checks are cheap. The remaining material costs are the roughly 256 MiB maximum Bloom probe plus legacy multi-column retention, and multi-GiB eager decoded-container resizing.
- Tests and results: current tests miss compact/binary string and container branches, count-within-input decoded amplification, contradictory exact Bloom lengths, and a genuine successful short payload read. No local build or tests were run, per the review task. At review time, macOS BE UT, performance, and COMPILE checks are failing; Doris BE UT and code-review are pending; formatting, license, FE UT, and Cloud UT checks pass.
- Observability: existing Bloom warnings/counters are adequate for rejected metadata but do not enforce task admission; no separate observability-only finding was identified.
- Persistence, transactions, data writes, and FE/BE variables: none of these paths or interfaces is changed.
- User focus: no additional focus points were supplied; the complete PR was reviewed.
Round 3 converged: all three final passes returned NO_NEW_VALUABLE_FINDINGS, and the five inline comments below are the deduplicated final set.
| return Status::Corruption("Parquet bloom filter range exceeds file size"); | ||
| } | ||
| if (column_meta_data.__isset.bloom_filter_length && | ||
| (static_cast<uint64_t>(column_meta_data.bloom_filter_length) < total_size || |
There was a problem hiding this comment.
[P1] Require the declared Bloom length to match the header
bloom_filter_length is the exact size of the serialized header plus bitset, not just an upper bound. For example, metadata can declare header_size + 64 while the header says numBytes = 32; this condition accepts it, reads only the first block, and initializes a one-block filter. If the payload was built as two blocks and a present value landed in block 1, test_hash() recomputes its bucket for one block, sees the empty first block, and can falsely prune the row group. Please reject any present declared length that is not exactly header_size + numBytes, apply the same fix to validate_native_bloom_filter_layout(), and cover the contradictory layout end to end.
| std::vector<uint8_t> data_buffer(t_bloom_filter_header.numBytes); | ||
| // Validate the full split-block layout before allocating or adding metadata-controlled | ||
| // offsets; the Bloom filter implementation assumes complete 32-byte blocks. | ||
| std::vector<uint8_t> data_buffer(static_cast<size_t>(payload_size)); |
There was a problem hiding this comment.
[P1] Keep maximum-size Bloom filters inside the task memory budget
A valid numBytes == MAXIMUM_BYTES creates a 128 MiB std::vector here, then ParquetBlockSplitBloomFilter::init() deep-copies it into another new char[128 MiB]. Neither allocation uses Doris's allocator or try_reserve(), and _process_column_stat_filter() can retain one 128 MiB filter per distinct predicate column; several parallel scanners can therefore exceed the query/workload-group limit and OOM the BE even though every individual filter passes this new cap. Please reserve and allocator-track both temporary and retained bytes (preferably transfer one tracked buffer instead of copying it), and bound or avoid aggregate retention of very large filters. The format_v2 path has the same allocation pattern.
| const int32_t size_limit = | ||
| *len == 0 ? 1 | ||
| : static_cast<int32_t>(std::min<uint32_t>( | ||
| *len, static_cast<uint32_t>(std::numeric_limits<int32_t>::max()))); |
There was a problem hiding this comment.
[P1] Bound decoded container allocation, not only element count
container_limit = size_limit still lets a modest input allocate far more than its wire size. Thrift 0.16 generated list readers call resize(count) immediately after readListBegin(), while compact and binary preflight treat T_STRUCT as zero minimum wire bytes. Under the existing 100 MiB v2 footer cap, metadata can declare nearly 100 million SchemaElements (count <= len), pass this guard, and resize the decoded vector to multiple GiB on the current ABI before reading or validating the first element; padding makes the transport window large enough, and failure afterward is too late. Please enforce a decoded or reservation-aware container budget before generated resize, and add compact and binary regressions where the count is within the input length but decoded storage exceeds the allowed budget.
| ParquetStatisticsTest() = default; | ||
| }; | ||
|
|
||
| TEST_F(ParquetStatisticsTest, reject_truncated_bloom_filter_payload) { |
There was a problem hiding this comment.
[P2] Make this test reach the short-read check
This fake reports _data.size() as the file size, so with a 64-byte header claim and only 32 payload bytes the production code returns from total_size > available before it calls the payload read_at(). The test would still pass if the new bytes_read != data_buffer.size() guard were removed. Please let the fake advertise the full logical range while forcing a successful short payload read, assert that no Bloom filter is installed, and keep the current range-truncation scenario as a separate case.
| @@ -71,6 +72,19 @@ class ParquetThriftReaderTest : public testing::Test { | |||
| void TearDown() override { TimezoneUtils::clear_timezone_caches(); } | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
[P2] Cover the other newly bounded Thrift paths
This only exercises a compact-protocol container length, but the production change separately constructs the binary protocol and enables both string_limit and container_limit in both branches. A regression that drops or misorders the binary limit, or either string limit, would therefore leave this test green even though network-facing binary callers or Parquet string fields could again allocate from a hostile length. Please add focused malformed string/container cases for compact=true and compact=false, together with a valid control, so every newly enabled allocation guard is covered.
### What problem does this PR solve? Issue Number: None Problem Summary: Parquet Bloom readers could accept contradictory declared lengths, allocate duplicate untracked payload buffers, and retain multiple large filters. Thrift generated readers could resize decoded containers before task memory admission. This change requires exact Bloom ranges, reads into tracked single-owner buffers, caps legacy cache retention, and reserves decoded Thrift container storage before resize. ### Release note Reject malformed Parquet Bloom metadata conservatively and enforce task memory admission for metadata decoding. ### Check List (For Author) - Test: Unit Test - Behavior changed: Yes, malformed metadata now falls back conservatively and excessive decoded metadata is rejected before allocation. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes: the follow-up correctly fixes exact Bloom layout validation, tracked Bloom ownership, short-read handling, legacy cache retention, and basic compact/binary Thrift limits, but three P1 correctness/resource defects and two P2 error/coverage gaps remain.
Critical checkpoint conclusions:
- Goal and proof: partial. Valid external scanner opens can now fail before parsing, valid cached page headers can be rejected based on unrelated payload bytes, and the decoded-container budget remains incorrect in both directions. The new tests do not exercise these paths or the supported absent-
bloom_filter_lengthcompatibility branch. - Scope, clarity, and reuse: the Bloom changes are focused and consistently applied. The shared Thrift helper is the right common boundary for wire limits, but a protocol-level wire-tag heuristic is not an adequate boundary for schema-declared C++ allocation.
- Concurrency: no new shared mutable state, locks, or lock-order risk was introduced. The important thread interaction is the new unconditional Doris thread-context dependency on Apache Thrift-created pthreads and contextless bthread callbacks.
- Lifecycle and static initialization: Bloom derived/member/base destruction, tracked-buffer release, bvar accounting, reservation-token restoration, nesting, and exception unwind are balanced on the reviewed paths. No cycles or cross-TU static initialization dependency were introduced.
- Configuration: no configuration item was added or changed.
- Compatibility: compact/binary constructor flags and wire limits remain compatible, and production still supports older Parquet metadata without
bloom_filter_length. Forward-compatible unknown Thrift fields regress because genericskip()is charged for a container it never materializes, and the absent Bloom-length branch lacks regression coverage. - Parallel paths and conditions: v1 and v2 Bloom readers now agree on exact layout, range validation, tracked direct reads, short reads, and conservative fallback. Binary/compact and mandatory/optional Thrift callers were traced; the page-cache upper-bound window, page-header retry loops, threaded service entry, generated vector resize, and unknown-field skip are the remaining bad branches.
- Tests and results: the added BE tests cover present exact Bloom lengths, true short reads, malformed strings/containers, and basic valid controls, but miss the five accepted scenarios. No local build or test was run because the review prompt explicitly prohibits it; no generated result file was changed.
- Observability and errors: existing Bloom warnings/counters are adequate. Memory-admission statuses are currently flattened to
InternalError, retried as parse failures, and finally misreported asIOError, which is the separate P2 error-semantics finding. - Persistence, transactions, data writes, and FE/BE variables: none are changed by this PR; no visibility, delete-bitmap, EditLog, atomicity, or variable-propagation concern applies.
- Performance and memory: direct tracked Bloom reads and the 16 MiB legacy retention cap are improvements. Reserving an entire cached payload for a header parse and retrying memory failures adds avoidable admission/I/O cost, while wire-tag pricing still permits real decoded amplification.
- Other issues: every candidate and initial risk was independently verified and deduplicated against the five existing inline threads. No additional configuration, correctness, lifecycle, or observability issue survived the final sweep.
- User focus: no additional focus points were supplied; the complete 12-file authoritative diff was reviewed.
Round 3 converged: both normal full-review tracks and the separate risk-focused track returned NO_NEW_VALUABLE_FINDINGS. The five inline comments below are the complete deduplicated final set.
| MemoryBudgetProtocol(std::shared_ptr<apache::thrift::protocol::TProtocol> protocol, | ||
| int32_t serialized_size) | ||
| : TProtocolDecorator(std::move(protocol)), | ||
| _memory_manager(thread_context()->thread_mem_tracker_mgr.get()), |
There was a problem hiding this comment.
[P1] Keep this utility valid on contextless service threads
MemoryBudgetProtocol now calls thread_context() before reading anything, but the backend open_scanner service uses ThriftServer's default THREADED mode. Those workers come from Apache Thrift's ThreadFactory, and the server event hooks install only _session_key, not a Doris ThreadLocalHandle. Consequently every valid open_scanner() plan reaches this line without a context, throws, and is returned as an invalid scanner open. Please make absent-context calls use a legitimate limiter (merely creating TLS leaves the orphan tracker and still fails the reservation invariant), preserve any attached task tracker, and add a real contextless threaded-service regression.
| : TProtocolDecorator(std::move(protocol)), | ||
| _memory_manager(thread_context()->thread_mem_tracker_mgr.get()), | ||
| _prior_reservation(_memory_manager->take_reserved_memory()) { | ||
| reserve_or_throw(static_cast<size_t>(serialized_size), /*restore_prior_on_failure=*/true); |
There was a problem hiding this comment.
[P1] Do not reserve the caller's whole readable window
len is only an upper bound on the encoded object (the helper replaces it with the bytes actually consumed), but both Parquet page-cache hit paths pass a cache entry containing the tiny Thrift header plus level data and the full compressed/decompressed payload. Reserving all serialized_size bytes here can therefore reject a valid cached page before parsing its header whenever the query has enough memory for the header but less than the already-cached payload size. Please separate the exact decoded-allocation budget from the readable window, or have these callers pass a bounded header-only view, and cover a valid small header followed by a large cached payload under a low task limit.
| uint32_t readListBegin_virt(apache::thrift::protocol::TType& element_type, | ||
| uint32_t& size) override { | ||
| const uint32_t consumed = TProtocolDecorator::readListBegin_virt(element_type, size); | ||
| reserve_container(size, decoded_thrift_value_reservation(element_type)); |
There was a problem hiding this comment.
[P1] Move decoded admission to the actual allocation site
This hook cannot infer decoded storage from the wire tag. Generated readers resize their statically declared C++ vector regardless of the returned element_type, so list<TPlanNode> can advertise T_BOOL, reserve one byte per element here, and allocate count * sizeof(TPlanNode) before parsing. The opposite path also fails: Thrift 0.16's generic skip() reaches this virtual hook for unknown fields but constructs no container, so a valid forward-compatible list<empty struct> is charged 1 KiB per element and can be rejected for phantom memory. Please enforce admission where the actual generated allocation/target type is known (or at allocator resize), leave skip traversal charged only for storage it creates, and test both a forged tag with a real large struct and a large unknown container.
| if (restore_prior_on_failure) { | ||
| _memory_manager->adopt_reserved_memory(std::move(_prior_reservation)); | ||
| } | ||
| throw apache::thrift::protocol::TProtocolException( |
There was a problem hiding this comment.
[P2] Preserve memory-limit failures across this boundary
try_reserve() returns specific query, workload-group, or process memory errors, but wrapping only status.to_string() in a TProtocolException makes deserialize_thrift_msg() return InternalError. Both Parquet page-header loops then treat that as an incomplete parse, retry with progressively larger reads, and finally replace it with IOError; other mandatory callers also lose the actionable status. Please carry the original Doris Status through the protocol boundary and return it unchanged, reserving deserialization errors for actual wire failures, with a regression that proves page readers do not retry memory-pressure failures.
| .ok()); | ||
| } | ||
|
|
||
| TEST_F(ParquetStatisticsTest, accept_valid_bloom_filter_layout) { |
There was a problem hiding this comment.
[P2] Cover Bloom metadata without the optional length
The local Parquet definition says bloom_filter_length was added in 2.10 and may be absent in older files, and both changed readers retain a separate compatibility branch for that case. Every v1/v2 Bloom fixture sets the field, including this valid control, so an accidental dependency on it can break old/external files while the suite stays green. Please add row-group cases with a valid offset, no declared length, and trailing file bytes, proving both a present value is retained and an absent value can still prune.
### What problem does this PR solve? Issue Number: None Problem Summary: Thrift container admission used wire types instead of generated target allocations, contextless service workers lacked a valid limiter, readable windows were over-reserved, and memory-limit failures were retried as parse errors. This change instruments generated container resize sites, preserves task trackers and memory statuses, avoids retrying memory failures, and adds compatibility coverage for Bloom filters without declared lengths. ### Release note Make Parquet metadata deserialization memory admission accurate and preserve actionable memory-limit failures. ### Check List (For Author) - Test: Unit Test - Behavior changed: Yes, decoded Thrift containers are admitted at their actual allocation sites and memory failures are returned without parse retries. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes: the patch now fixes the previously reported Bloom-layout, ownership, short-read, compatibility-test, and basic Thrift-limit gaps, but two P1 resource-accounting defects remain in the current head.
Critical checkpoint conclusions:
- Goal and proof: partial. Bloom metadata is now read into one tracked owner and validated conservatively, and generated vector resizes receive a per-call admission check on clean builds. However, incremental builds can leave most generated readers without that check, and successful decoded storage loses its charge while still live. The tests do not prove either missing invariant.
- Scope, clarity, conditions, and error handling: the production change is focused and the common Thrift boundary is appropriate. The new size/overflow checks and v1 Bloom/page-header conditions are otherwise coherent; the remaining defects are the incomplete Make dependency and the unconditional successful-reservation release.
- Concurrency: no new shared mutable state, locks, lock-order risk, or race was found. Scanner and service-thread deserializations are independent, but parallel callers amplify the untracked live decoded storage.
- Lifecycle and static initialization: Bloom derived/member/base destruction, payload ownership, cache eviction, failure unwind, and counters are balanced, with no cycle or cross-TU static-order issue. The decoded-container lifecycle is not balanced:
MemoryBudgetProtocoldrops the reservation before the returned standard containers die. - Configuration: no configuration item was added or changed. Existing task and workload-group limits are the affected policy; dynamic configuration propagation is not applicable.
- Compatibility: no Thrift wire, Parquet storage, or FE/BE protocol change was introduced, and legacy Bloom metadata without
bloom_filter_lengthremains supported. Rolling compatibility is otherwise unaffected, but normal incremental source generation is not fresh for non-Parquet readers. - Parallel callers and implementations: v1/v2 Bloom ownership and layout behavior are aligned. The Make defect affects live plan, descriptor, profile, and service-support
_types.cppreaders, while the retained-output defect is concretely reachable in the v2 multi-column page-index loader. Concerns already covered by the ten live threads were not repeated. - Tests and results: the changed BE tests cover many compact/binary, page-header, and Bloom branches, but miss incremental reinstrumentation and aggregate accounting across two retained successful outputs. No local build or tests were run because the review prompt prohibits them. At submission time, macOS BE UT is failing; Doris BE UT, COMPILE, Cloud UT, FE UT, code-review, and performance are pending; formatter, CheckStyle, license, secret, dependency-review, and related lightweight checks pass.
- Observability: Bloom warnings, timers, and fallback counters are adequate. Successful standard-container storage is invisible to task/workload-group accounting after return, which is part of the accepted lifecycle defect rather than a separate logging issue.
- Persistence, transactions, data writes, and FE/BE variables: none of these surfaces is changed, so no durability, atomicity, visibility, or variable-propagation issue applies.
- Performance and memory: direct Bloom payload reads remove the previous extra copy and retention is bounded. The remaining material risks are stale uninstrumented generated readers and repeated retained decoded objects whose aggregate bytes escape task/workload-group limits; no separate CPU regression survived review.
- User focus: no additional focus points were supplied; all 17 authoritative changed files and their relevant callers, parallel paths, and tests were reviewed.
Round 3 converged: all three final passes returned NO_NEW_VALUABLE_FINDINGS. The two inline comments below are the complete deduplicated final set.
| ${BUILD_DIR}/gen_cpp: | ||
| mkdir -p $@ | ||
| # handwrite thrift | ||
| ${BUILD_DIR}/gen_cpp/parquet_types.cpp: ${CONTAINER_MEMORY_CHECK} |
There was a problem hiding this comment.
[P1] Regenerate every reader that needs this hook
The checker is a prerequisite only of parquet_types.cpp, while the pattern targets for Types, PlanNodes, PaloInternalService, RuntimeProfile, and the other generated readers still depend only on their unchanged IDL. The normal build.sh path calls generated-source.sh noclean, so after an incremental checkout those existing outputs remain newer and Make never reruns this recipe; their deserialize_thrift_msg() callers therefore use MemoryBudgetProtocol with generated read() methods that contain no reserve_thrift_container_memory() call, leaving the original eager large-struct resize/OOM path reachable in plan and service requests. Please make the checker a normal prerequisite of every instrumented %_types.cpp target, and cover an incremental regeneration without --clean.
| _prior_reservation = _memory_manager->take_reserved_memory(); | ||
| } | ||
|
|
||
| ~MemoryBudgetProtocol() override { |
There was a problem hiding this comment.
[P1] Keep successful decoded storage charged for its lifetime
These generated fields are ordinary std::vectors, and the current malloc/new paths do not call ThreadMemTrackerMgr::consume(). Their resizes therefore never convert _reserved_mem into persistent consumption, yet this destructor releases the full reservation while deserialized_msg still owns the vectors. load_native_page_indexes() repeats this for every selected column and retains all of the decoded ColumnIndex/OffsetIndex objects, so each call can pass the task/workload-group checks independently while their aggregate live storage exceeds those budgets. Please make successful decoded storage allocator-tracked or carry an owned charge for its actual lifetime, shrink only unused reservation, and add a test that retains two individually admissible outputs whose sum exceeds the limit.
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Requesting changes: five P1 memory-admission/correctness defects and three P2 build/test gaps remain. The third and final review round converged: all three normal full-scope tracks and all three independent risk challenges returned NO_NEW_VALUABLE_FINDINGS, all nine subagent candidates were resolved, and the eight accepted findings below are distinct from the 12 existing live comments.
Critical checkpoint conclusions:
- Goal and scope: The PR partially achieves bounded Thrift/Parquet allocation and safer Bloom reads, and the production changes are focused on those paths. It does not yet bound generated map/set allocation, preserve accurate admission across copies/reallocation/bookkeeping, or make the post-generation state reliable.
- Concurrency and thread safety: Contextless pthread/bthread attachment, nested tracker restoration, cross-thread token release, and per-scanner Bloom ownership were traced and are otherwise balanced. No new shared-state, lock-order, atomic-ordering, or deadlock defect was found.
- Lifecycle and memory safety: Bloom buffers have single tracked ownership and balanced derived/member/base destruction. The retained Thrift container lifecycle still fails at wrapper copies, repeated-capacity growth, and nested zero-byte bookkeeping (MAIN-001/003/004), while map/set nodes bypass admission entirely (MAIN-002).
- Error handling: Reservation and parse failure unwind was checked. The already-live status-preservation/page-header retry issue was not duplicated; no additional distinct error-boundary issue remained.
- Compatibility and parallel paths: Wire/storage formats and FE-BE protocol values are unchanged. Both v1 and FileScannerV2 Bloom/page-index paths, compact/binary Thrift, all current IDLs, and clean/noclean generation were reviewed. Bloom pruning remains conservative on malformed/absent/truncated inputs, but the generator/build paths have MAIN-005/008.
- Conditions and tests: Exact/absent Bloom length, v1 successful short reads, ordering, context attachment, and retained-output cases were inspected. The new Python test is not registered with a normal test target (MAIN-006), and the v2 short-read fixture exits before the changed guard (MAIN-007). No local build or test was run because the authoritative review instructions prohibit it.
- Configuration, persistence, transactions, and data writes: No configuration item, persisted state, transaction path, data-write behavior, or cross-layer variable propagation is introduced.
- Performance and observability: The 16 MiB v1 Bloom cache cap and v2 one-filter-at-a-time behavior are sound; counters/logging remain adequate. MAIN-003 permits O(N) unadmitted bookkeeping, and MAIN-005 causes perpetual no-op regeneration on affected noclean builds.
- User focus: No additional user-provided review focus.
Review status: converged and complete for the bundled head. Requesting changes on the eight inline findings.
| using Base::operator=; | ||
|
|
||
| ThriftMemoryTrackedVector() = default; | ||
| ThriftMemoryTrackedVector(const ThriftMemoryTrackedVector&) = default; |
There was a problem hiding this comment.
[P1] Do not share one reservation across deep copies. These default copy operations deep-copy the std::vector storage but only copy _memory_charge. The v1 page-index path reaches this directly: after decoding a charged OffsetIndex, vparquet_reader.cpp:1233 lvalue-copies it into _col_offsets, so two large buffers coexist while only one is admitted. Lines 1322-1323 also copy charged min_values/max_values into ordinary vectors whose storage outlives the source charge. Please make copied storage acquire its own admission/charge (or make these transfers move-only and preserve ownership), and add a limit test that copies one admissible decoded index under headroom below the two-copy peak.
|
|
||
|
|
||
| INCLUDE = '#include "util/thrift_container_size.h"' | ||
| RESIZE = re.compile(r"^(?P<indent>\s*)(?P<container>.+)\.resize\((?P<size>_size\d*)\);$") |
There was a problem hiding this comment.
[P1] Cover generated maps and sets as well as list resizes. This is the only source pattern the postprocessor instruments, but Thrift 0.16 fills maps with operator[] and sets with insert, so those node allocations never call reserve_thrift_container_memory(). This is reachable in the contextless plan-fragment deserialize path (TPipelineFragmentParamsList contains several scalar maps), where a compact wire map can expand into much larger std::map nodes without either task or fallback-process admission. Please instrument these generated allocation loops using the declared target type and add compact/binary map and set budget regressions.
| if (!status.ok()) { | ||
| throw Exception(status); | ||
| } | ||
| return std::make_shared<ReservedMemoryCharge>(_memory_manager->take_reserved_memory()); |
There was a problem hiding this comment.
[P1] Avoid one uncharged heap token per nested container. reserve_container_memory(0, ...) still reaches this make_shared, and ordinary generated vectors then append another shared_ptr to _temporary_charges. For a reachable list<list<...>> such as TRepeatNode.grouping_list, N empty children admit only the outer N * sizeof(vector) storage but allocate N control blocks/tokens plus N temporary-vector entries outside that admission, defeating the boundary by another O(N) amount. Please aggregate temporary reservations (and special-case zero bytes), or include this bookkeeping in admission; cover many empty nested lists under constrained headroom.
| // The generated target type, rather than the untrusted wire tag, defines the allocation made | ||
| // by vector::resize. Unknown fields never reach this generated allocation hook. | ||
| if (auto* checker = dynamic_cast<ThriftContainerMemoryChecker*>(protocol); checker != nullptr) { | ||
| const size_t elements = std::max<size_t>(count, container->capacity()); |
There was a problem hiding this comment.
[P1] Budget the allocation that resize will actually make, including reallocation overlap. Thrift accepts repeated field IDs, so an external page index can present a tracked list first with N entries and then N+1. This hook replaces the N-element charge with only max(N+1, capacity)==N+1; on current libstdc++, the following resize grows capacity to 2N while the old N buffer is still live. The result is 2N steady / 3N transient storage admitted as N+1. Please tie the charge to actual allocator capacity/peak (without releasing the old charge early) and add repeated-list compact/binary cases.
| # handwrite thrift | ||
| ${BUILD_DIR}/gen_cpp/%_types.cpp: ${CURDIR}/%.thrift | ${BUILD_DIR}/gen_cpp | ||
|
|
||
| ${BUILD_DIR}/gen_cpp/%_types.cpp: ${CURDIR}/%.thrift ${CONTAINER_MEMORY_CHECK} | ${BUILD_DIR}/gen_cpp |
There was a problem hiding this comment.
[P2] Avoid leaving no-op generated targets permanently stale. After this checker changes in an incremental checkout, this prerequisite reruns every target; Thrift 0.16 preserves an existing _types.cpp mtime when its content is unchanged, and the postprocessor also does not write when it finds no resize. IDLs such as Metrics.thrift, QueryCache.thrift, and QueryPlanExtra.thrift therefore remain older than the checker and recompile on every later generated-source.sh noclean. Please complete the rule with a stamp/mtime strategy for no-op outputs and test that a second noclean pass does no work.
| CHECKER = SCRIPT_DIR / "add_container_memory_check.py" | ||
|
|
||
|
|
||
| class AddContainerMemoryCheckTest(unittest.TestCase): |
There was a problem hiding this comment.
[P2] Register this test with a normal repository test target. No gensrc Make target, build.sh/run-*-ut.sh path, build-support test, or checked-in workflow invokes this file; the only entry point is the direct unittest.main() block. As added, both the postprocessor-shape assertion and the Make prerequisite assertion can regress while standard CI remains green. Please wire it into an executed target, then extend it with the real two-pass noclean case.
| @@ -1276,7 +1293,65 @@ TEST(ParquetBloomFilterPruningTest, NativeBloomReportsConservativeReadOutcomes) | |||
| auto truncated = make_valid_bloom(); | |||
There was a problem hiding this comment.
[P2] Make this fixture reach the v2 successful-short-read guard. StatisticsMemoryFileReader::size() equals _bytes.size() and its read either fills the whole request or returns IOError; after this shrink, validate_native_bloom_filter_layout() rejects header_size + numBytes > file_size before the payload read. Thus deleting the new bytes_read != bloom_filter->size() check still leaves the test green. Let the fake advertise the validated logical range while returning a shorter successful payload read, assert that branch is reached, and keep this range-truncation case separately.
| # handwrite thrift | ||
| ${BUILD_DIR}/gen_cpp/%_types.cpp: ${CURDIR}/%.thrift | ${BUILD_DIR}/gen_cpp | ||
|
|
||
| ${BUILD_DIR}/gen_cpp/%_types.cpp: ${CURDIR}/%.thrift ${CONTAINER_MEMORY_CHECK} | ${BUILD_DIR}/gen_cpp |
There was a problem hiding this comment.
[P1] Make this source/header transformation failure-atomic. The recipe tracks only _types.cpp, while the checker writes that source first and its sibling header second. If header processing exits nonzero after the source write, Make keeps the newly dated .cpp; after the transient cause is repaired, the normal noclean retry sees this sole target newer than the IDL/checker and skips both transformations. The ordinary-vector header is still compile-compatible with the source hook, so retained page-index fields silently fall back to protocol-lifetime charging. Please use an atomic/grouped completion stamp (or delete/invalidate the target on failure) and test a failure between the two writes followed by a noclean retry.
…count probe (FE) (#66831) ### What Two independent breakages that each make **current master fail to build** — one in BE configure, one in FE compile. They share a shape: a pair of PRs that never conflict textually, merge cleanly, and only break once combined, so each PR's own pipeline was green. | | Breakage | Colliding PRs | |---|---|---| | BE | `cmake` configure aborts | #66052 moved a file, #66789 made a dangling unity-skip entry fail loud | | FE | `fe-connector-iceberg` does not compile | #66778 deleted `getCountFromSnapshot()`, #66413 added a caller for it | CI merges each PR into the latest master before building, so **every PR pipeline that picks up current master is red** on one or both. --- ## 1. BE — stale `STORAGE_UNITY_SKIP` entry for a moved file Remove the stale `STORAGE_UNITY_SKIP` entry (and its comment block) for `compaction/collection_statistics.cpp`, which no longer exists. ### Why — master configure is currently broken #66052 moved `storage/compaction/collection_statistics.{cpp,h}` to `storage/index/inverted/similarity/` (rewritten), but left behind the unity-skip entry that #66789 had added for the old path. The fail-loud validation introduced by #66789 turns a dangling skip entry into a configure-time error — which is exactly what it is designed to catch (a skip list rotting after a file move), so BE configure on current master fails immediately: ``` CMake Error at CMakeLists.txt:1002 (message): unity skip entry does not exist (renamed or moved?): .../be/src/storage/compaction/collection_statistics.cpp ``` #66826, #66824, #66819, #66820 were the first hits — same error on multiple independent agents. ### Why deletion (not a path update) is correct The old entry existed because the old `collection_statistics_test` `#include`d the `.cpp` into a second TU (unity batching would then produce a duplicate definition at link time). The rewritten file at the new location is not `#include`d by any test (`grep -rn 'collection_statistics.cpp' be/test/` is empty on master), so the new path needs no skip entry. ### Verification - Full BE build from a clean tree at master + this change (clang20 / macOS arm64, unity=ON, PCH=ON): configure passes the skip-list validation and the build compiles. (The same tree without this change fails configure with the error above.) - Timeline note: #66052's last green CI round presumably predates #66789's validation landing (2026-08-16), which is how the dangling entry slipped through. --- ## 2. FE — the metadata-only COUNT(\*) probe calls a deleted method #66778 replaced the snapshot-summary COUNT(\*) pushdown with a manifest-derived count and deleted `getCountFromSnapshot()`. `canServeMetadataOnlyCount()`, added by #66413, still calls it, so FE compilation fails: ``` [ERROR] .../connector/iceberg/IcebergScanPlanProvider.java:[505,16] cannot find symbol [ERROR] symbol: method getCountFromSnapshot(org.apache.iceberg.TableScan,org.apache.doris.connector.spi.ConnectorSession) [ERROR] location: class org.apache.doris.connector.iceberg.IcebergScanPlanProvider ``` ### Why re-express the probe instead of restoring the method Bringing `getCountFromSnapshot()` back would reintroduce precisely what #66778 removed: a query result derived from optional, writer-provided snapshot summary fields. The probe is rebuilt in #66778's own terms instead. It reuses that PR's delete gate and additionally requires the data manifests to carry aggregate row counters, so the answer is proved from the manifest list alone — `O(manifests)`, no data-file enumeration, which is what a probe running before planning can afford. Manifest lists that omit those aggregates now answer `false`, where count planning still serves them through its bounded per-file fallback. A capability probe should under-promise rather than over-promise; the planner itself is untouched. ### Verification - `mvn package` over the full FE reactor (74 modules): all green. This also confirms no second semantic break is hiding behind the first — CI's maven stops at `fe-connector-iceberg` and never reaches the rest. - `IcebergScanPlanProviderTest`: 153 tests, 0 failures, including a new case pinning that the probe follows the same delete gate as count planning. - FE checkstyle: 0 violations. ### Release note None (both fixes only restore a buildable master; no user-visible behavior change). ### Check List (For Author) - Test - [x] Regression test — not applicable: neither change alters query behavior - [x] Unit test: `IcebergScanPlanProviderTest` (153 tests, 0 failures) - [x] Manual test: full BE configure/build and full FE reactor build - Behavior changed: No - Does this need documentation: No --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2 participants
What problem does this PR solve?
Malformed Parquet metadata can encode internal Thrift container lengths that are much larger than the serialized message, causing generated readers to allocate excessive memory before deserialization fails.
The legacy Parquet Bloom filter reader also trusts metadata-controlled offsets and sizes before allocation. A malformed filter can therefore cause excessive allocation, out-of-bounds block access, or incorrect pruning when a payload is truncated.
What is changed?
Check list