Skip to content

feat(planning): add finite-byte admission for retained snapshots - #462

Open
realAaronWu wants to merge 6 commits into
apache:mainfrom
realAaronWu:codex/paimon-full-planning-budget
Open

realAaronWu wants to merge 6 commits into
apache:mainfrom
realAaronWu:codex/paimon-full-planning-budget

Conversation

@realAaronWu

@realAaronWu realAaronWu commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

Ordinary metadata planning does not admit every variable allocation against a caller-owned byte limit. Add opt-in TableScan::CreateWithPlanningBudget so finite-budget callers can reject allocations before requesting storage, including allocations outside MemoryPool. Exhaustion returns CapacityError; allocation failure returns OutOfMemory. No failure returns a partial plan or invokes unrestricted planning.

The budget counts cumulative admitted requested bytes. Frees do not replenish it, so temporary input, parser, decompression and publication storage cannot each reuse the whole limit. It covers retained caller schema/path/options, scan/plan objects, metadata buffers, JSON DOM/parser stacks, Avro schema/records, Zstandard workspace/output, native metadata, strings, split vectors and ownership blocks. Fixed diagnostics/control objects, immutable process constants and existing shared services are excluded. This is not an RSS or peak-live measurement. Escaped nonempty splits retain the budget and allocator. Overlapping operations and escaped-owner cleanup require a thread-safe upstream allocator, such as the default pool, or caller synchronization; individual scans are not concurrent.

The initial admitted profile is explicit: Linux/libstdc++9+ C++11 ABI, compatible static Zstandard, retained schema, positive numeric snapshot ID, local unpartitioned append-only bucket-unaware tables, matching file schema, version-2 ADD-only Avro manifests, null/Zstandard compression and Parquet/ORC files. Unknown decoded lengths use admitted streaming workspace/output growth. Unsupported contexts/profiles, DELETE entries, duplicates, wrong writer types and malformed inputs fail explicitly. Other platforms/old ABI/GCC8/shared Zstandard select a compiled NotImplemented factory; ordinary scans retain their existing path. System-static Zstandard must pass a current-header/library API link probe, refreshed on reconfiguration, and runtime header/library versions must match.

Generic Avro/Arrow decoding does not route all variable storage through admission. This decoder uses budget-backed parser/PMR storage and static Zstandard workspace. Existing native publication constructors receive pre-admission based on qualified GNU string/vector/shared-pointer allocation behavior. The path is synchronous, emits ordinary native splits, and omits shared metadata caching and pruning statistics.

Related: sirius-db/sirius#2211 and polardb/duckdb-paimon#145. This independent native PR does not change the bridge or depend on cancellation PRs.

Tests

Final reviewed head: f6acf07c3597fe56377d10d275f837d817138e5a.

  • PASS: full native build on Linux x86_64/GNU15.3 with pinned bundled dependencies; all 13 budget/planner tests with ASan+UBSan, leak detection enabled and no suppression file. Independent final-head replay also passed.
  • PASS: Parquet full-field and ORC id-projection plan/read parity, independent row IDs, escaped-split reading, exact/insufficient/zero quotas, malformed and unsupported input rejection, and failure injection through every tested arena/publication allocation. ORC TIME decoding is an existing excluded reader limitation.
  • PASS: observed actual pool and ordinary operator new requests with long-string calibration; static test runtime includes out-of-line libstdc++ constructors. Four earlier assertion-specific mutations catch missing string admission, quota enforcement, JSON failure draining and writer-schema validation; affected logic remains unchanged in the final lint follow-up.
  • PASS: deterministic actual allocation/cross-thread free overlap plus shared allocation stress. Three budget-component cases repeated ten times under TSan with no diagnostic, and under actual GCC8.4/libstdc++8 and GCC9.3/libstdc++9 runtimes. All three new TUs also compile under strict GCC8/GCC9 warning settings; unsupported old-string-ABI implementation/test compilation passed.
  • PASS: GCC9 full native/dependency rebuild and all 13 final-head planner tests using static libstdc++9, with -Wno-error=sign-compare for an unchanged mainline warning in serialization_utils.h:85. The vanilla GCC9 -Werror build stops at that pre-existing warning and is not claimed to pass.
  • PASS: bundled/static/UNKNOWN-static/shared CMake target checks; same-build compatible-static to missing-symbol reconfiguration disables the bounded path. Actual cap0 scanner/test objects linked with shared Zstandard pass four cases including ordinary planning. Runtime version mismatch/match negative/positive check passed. These are targeted configuration and affected-TU checks, not a fresh full SYSTEM build.
  • PASS: final-head repository-wide pre-commit; direct Clang21 clang-tidy over the six affected source/public-header files; Apache RAT0.16.1 on a clean final source archive (Java26), zero unknown licenses. Independent source/compatibility/delivery review verified all 5,473 source hashes and final binaries and closed its findings.
  • Historical PASS: existing core suite 2,478/2,478 before compatibility/lint follow-ups; final changes do not modify ordinary core planning. A prior serial warm Debug 100-sample fixture comparison measured ordinary→bounded median Parquet 6.10→2.68 ms and ORC 2.74→1.49 ms. This is narrow fixture evidence, not Release/cold-cache/concurrent performance qualification.

Reproduction entry points: build paimon_shared and paimon-bounded-table-scan-test, run the registered ctest -R '^paimon-bounded-table-scan-test$' --output-on-failure, and run pre-commit run --all-files. Direct test runs require PAIMON_TEST_DATA pointing at test/test_data/ with a trailing slash.

CI scope: exact-head fork pre-commit and GCC8 passed. The GCC8 job executed 40 CTest targets, including the explicit unsupported-factory/ordinary-plan target. In Build and Test, completed ARM64 Clang/GNU Release/Debug and ASan+UBSan rows each passed 41 CTest targets; ARM64 TSan passed 39, and x86_64 TSan passed 40. Each included the bounded-planning target. The x86_64 GNU Release/Debug, Clang Release and ASan+UBSan rows have also completed: each passed 42 CTest targets including bounded planning, with exact-head checkout and actual logs independently audited. All twelve native matrix job results now pass. The corrected x86_64 Clang Debug job in attempt 2 passed all 42 CTest targets, including bounded planning, and the actual check-clang-tidy target. Its complete job log and exact f6 checkout were independently audited. Eleven earlier successful job results were carried forward; only Clang Debug was rerun. The first Clang Debug attempt passed 42 CTest targets but failed lint on logging.cpp, which is unchanged by this PR. Fork main was normally fast-forwarded from stale 242dec to actual upstream base a397 before the rerun, correcting origin/main comparison provenance without changing PR source or lint rules. That original failure remains historical evidence. These are CTest target counts, not individual GTest case counts. The existing workflow uses recoverable UBSan settings and retains no per-test artifacts, so instrumented job success is not an unqualified UBSan-clean claim; strict local final-head sanitizer evidence is separately recorded above. All four upstream PR-event workflows now pass on unchanged f6: Build and Test, GCC8, pre-commit and License. Independent review verified the actual PR merge/head identical tree, all twelve native matrix jobs plus script-tests and their successful steps without skips. Complete selected native logs confirm x64 Clang Release 42/42 targets and ARM Clang/GNU Release 41/41, each including bounded planning; complete GCC8 logs confirm 40/40 targets, and all nine pre-commit hooks passed. Nine other native terminal logs were not fully downloaded due network limits; their successful execution is verified by completed job metadata and the unchanged workflow, without claiming retained per-case reports. Maintainer code review/merge remains pending. Full old-ABI runtime and a fresh full SYSTEM build are NOT RUN. No current CI failure remains; maintainer review and merge remain separate.

API and Format

Add exported PlanningBudget and an additive static scan factory. Existing interfaces and persisted formats are unchanged. Native plan/split outputs use existing data readers. Reading, metrics, serialization and caller copies after planning require their own allocation policy.

Documentation

Add the bounded-planning guide covering contract, admitted/rejected profile, ownership/concurrency, allocation qualification, limitations and usage.

Generative AI tooling

Generated-by: OpenAI Codex (GPT-6)

@realAaronWu

realAaronWu commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor Author

@lucasfang all four upstream checks now pass on unchanged head f6acf07c3597fe56377d10d275f837d817138e5a:

Independent checks verified the tested PR merge has the reviewed head's exact tree, actual job/step completion and selected native/GCC8/pre-commit raw logs. The description distinguishes complete logs from successful metadata where transport limited downloads, and retains the sanitizer/support boundaries. Local/fork qualification and source-review findings are closed; no new source change or CI rerun is proposed.

Could you review the PR and merge it if the implementation and documented scope look acceptable? My account has no upstream merge permission. This updates the original approval request; CI approval is resolved. The dependent duckdb-paimon #145 also passes its final Linux SQL/distribution and Linux/macOS dynamic bridge CI, while its partial experimental scope remains separately tracked.

@realAaronWu

Copy link
Copy Markdown
Contributor Author

@lucasfang gentle follow-up: could you review this PR and approve/merge if the implementation and documented scope look good? Unchanged head f6acf07c now has all upstream checks passing, including all 12 native matrix jobs, GCC8, script-tests, pre-commit and license checks. The implementation and validation details are in the description and comment above. This supplies finite planning budgets used by duckdb-paimon#145. Thanks!

@lxy-9602

Copy link
Copy Markdown
Member

Thank you for your submission! We’ve been preparing for the 0.4 release recently and there’s a fair amount of regression work to work through. I’m afraid we may not be able to start the review this week, but we expect to begin next week. Thanks again for your patience and understanding!

@lxy-9602

lxy-9602 commented Nov 2, 2026

Copy link
Copy Markdown
Member

@realAaronWu Sorry for the late response, and thanks for working on this and for the detailed implementation and documentation.

I’m concerned about the maintenance cost of introducing a separate planning and metadata decoding path. Future changes to the manifest format, file metadata, or regular planning semantics would need to be reflected in both implementations, creating a risk of divergence.

I would prefer reusing the existing planning path and passing a budget-aware MemoryPool through ScanContextBuilder::WithMemoryPool(). The custom pool could throw std::bad_alloc when an allocation would exceed its budget, and the caller could catch and handle the exception outside the planning call. Of course, paimon-cpp primarily uses Status and Result for error handling, so existing paths may need adjustments to propagate allocation exceptions safely and clean up resources correctly. Such targeted fixes would be welcome.

Some internal container allocations would remain outside this accounting. If those allocations are relatively small and temporary objects are released promptly, I think that is a reasonable tradeoff, provided the API clearly documents that the budget covers pool-managed allocations rather than all planning memory. Accounting for every allocation—especially through assumptions about STL implementation details—seems to introduce substantial maintenance risk.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants