Repository navigation
Paimon blob 4.1 - #69175
Paimon blob 4.1#69175
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: No final review submission was declared Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Changes requested: three verified findings are reported inline: [P1] unbounded allocation for unknown-length external BLOBs in JNI inline mode; [P2] the new FILE JNI path exposes an off-heap VARBINARY entry-array leak; [P2] native companion-file reads omit scanner I/O and resource byte accounting. There were no existing inline threads or confirmed existing blocking comment IDs to carry forward.
Reviewed all 49 changed files at 0306190 through three rounds, using two complete-review scopes plus an independent risk reviewer. Checked Scanner/TableReader/FileReader/mapper responsibilities; global/file-local identities; projection, historical field IDs and missing/null/default values; native/JNI/Rust capability gates; residual predicates, deletes and COUNT/MIN/MAX; file-global row positions across filtering, byte splits and EOF; split/error/cancellation cleanup and ownership; BLOB v1 framing/index/CRC/bounds; known/unknown payload limits; immutable file/cache behavior; JNI const/nested/null/empty FILE layouts and both VARBINARY transfer layouts; performance, I/O accounting and profile publication. No additional row-ID join, latest-companion selection or predicate/aggregate correctness defect was verified. All visible code candidates and risk foci have explicit accepted or dismissed dispositions in the shared ledger. No additional user focus was supplied.
Validation was static source and fixture inspection only, including official Paimon 1.4.2 writer/reader sources matching the dependency version. Added BE/Java/FE tests and both Spark-fixture regression/golden pairs were inspected; no builds, tests or benchmarks were run, repository source files were not modified, and submodules were left uninitialized. Runtime gaps include persisted unknown-length external descriptors, repeated FILE allocation accounting, native BLOB I/O/profile assertions, forced anchor byte splits/multiple row groups, branch/time travel/new BLOB fields, real nested/external descriptor interoperability and remote/performance comparisons. Existing fixtures and golden results are not executed test evidence.
The review did not converge after the three-round cap. All three reviewers returned NO_NEW_VALUABLE_FINDINGS in the final round, and visible changed-code coverage is complete, but the required repository code-review skill was unavailable: review_skill_prompt.txt contains only a circular instruction to read the skill, and a full repository inventory found no SKILL.md. The required AGENTS.md files and their mandatory architecture/common/Parquet guides were applied, but unseen skill checkpoints cannot be certified. This instruction-artifact coverage gap remains unresolved; all verified findings are delivered now under the bounded delivery contract.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
The required repository code-review skill is absent: review_skill_prompt.txt contains only a circular instruction and a complete repository inventory found no SKILL.md. Required AGENTS and visible mandatory guides were applied, but coverage of unseen skill checkpoints cannot be verified. All final-round reviewers returned NO_NEW_VALUABLE_FINDINGS, all 49 changed files and visible risk foci were covered, and no production candidate remains unresolved. Static-only runtime and benchmark limits are disclosed in the summary.
| new ColumnType("size", Type.BIGINT), | ||
| new ColumnType("content_type", Type.STRING), | ||
| new ColumnType("checksum", Type.STRING), | ||
| new ColumnType("inline", Type.VARBINARY)); |
There was a problem hiding this comment.
[P2] Release the entry buffer owned by the new FILE child
Every FILE now creates this VARBINARY child, including descriptor-only and SQL-NULL output. Its reserveCapacity() allocates 16 * capacity bytes, but the existing VectorColumn.close() calls freeVarbinaryData(), which frees only payload pointers, then clears data without freeing that array. JniTableReader releases and recreates the Java table for every batch, so the new Paimon BLOB-to-FILE path permanently leaks at least roughly 16 bytes per row per FILE (about 1.6 GB for 100 million rows), even without inline payloads. Free the owned VARBINARY entry array after its payloads and cover repeated FILE batch release.
|
/review |
There was a problem hiding this comment.
Reviewed head dd1aba0 across all 49 changed files in three bounded static-review rounds. Three verified P2 findings are inline:
- Descriptor, SQL NULL, and empty FILE payloads leak separately allocated zero-length native buffers.
- The JNI descriptor-mode copy can rewind an audit_log reader's bound schema after a schema-only ADD COLUMN and fail projection.
- Native managed-BLOB routing loses renewable REST-token credential refresh and can fail after token expiry.
The three supplied existing findings are independently confirmed fixed at this head: unknown-length BLOB reads are bounded and closed before retention; writable VARBINARY entry arrays are freed; companion reads use tracing and flush their I/O profile. None is reposted, and no existing P0/P1 finding is carried as blocking.
Required checkpoints covered layer ownership, mapper identities and FILE projection, historical field IDs and latest field-bunch selection, missing/null/empty values, original row positions through pruning/filtering/byte splits, residual/deletion and aggregate ordering, split/EOF/error/cancellation cleanup, buffer ownership, corruption/overflow/CRC bounds, size-before-allocation, caches, I/O accounting, and native/JNI/Rust/v1 eligibility. The managed-BLOB v1 parser and relevant SDK behavior were compared with the pinned Paimon 1.4.2 sources. Spark fixture installation, BE/Java test sources, both regression modes and expected outputs were inspected. No additional user focus was supplied.
Converged=false after round 3. All three latest reviewers returned NO_NEW_VALUABLE_FINDINGS against the frozen inline set, but strict convergence remains unproven. M6 remains an open performance question: uncached native payload reads issue per-record S3 requests; the comparative workload impact versus the bounded SDK/JNI paths was not established. This is excluded from inline defects. The required standalone code-review skill is absent from the checkout; the supplied review_skill_prompt only repeats its requirement. Required AGENTS.md files and their design/common/Parquet guides were applied.
Validation was static only: no builds, tests, benchmarks, source edits, or submodule initialization. End-to-end persisted unknown-length external descriptors, supported nested external BLOB interoperability, true multi-rowgroup byte splits, schema-only audit_log ALTER, and expiring-token execution remain unverified integration coverage. Generated goldens and source inspection are not executed test evidence.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
Round 3 cap reached. Strict convergence is unproven despite all latest reviewers returning NO_NEW_VALUABLE_FINDINGS: M6 comparative small-payload object-store performance remains unresolved, and the required standalone repository code-review skill is absent. No changed file is uncovered (49 reviewed); external/nested descriptor interoperability, actual multi-rowgroup byte splits, schema-only system-table evolution and renewable-token execution remain unverified integration coverage under the static-only contract. All three accepted findings are verified and delivered inline.
What problem does this PR solve?
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)