[Mryange]

Background

Tablet write validation can truncate strings, round legacy decimals, and deduplicate map keys. Expression outputs can share column trees, so validation must preserve copy-on-write ownership before changing them.

What problem does this PR solve?

Issue Number: None
Related PR: #70421 (superseded)
PR Type: Refactor

Problem Summary:

Tablet validation is embedded in the block converter as a large recursive function that mixes type dispatch, nullable handling, row filtering, and mutation. Extract it into a standalone, testable column validator with per-type helpers and a shared validation context. The new entry reads an immutable input tree and returns a validated column. Scalar checks reuse input storage; complex types build new shells around validated children, avoiding shared-column corruption and full-buffer copies for read-only validation.

Details

• Change map: Roughly 500 lines of embedded validation are replaced by a standalone implementation and interface; the converter keeps block orchestration. Add ownership/validation UTs and a 55-line regression with generated output. No new column mutation accessor is needed.
• Moves vs. logic changes: Move scalar checks and decimal bound caches into per-type helpers/context. Centralize physical nullable handling and original-row mapping. Replace const-cast mutation with immutable recursive traversal and fresh container shells. String truncation returns a replacement; DecimalV2 clones only if rounding is needed. Each new map shell deduplicates once, detaching child data only when filtering duplicates.
• Behavior change: Validation no longer modifies shared source columns or retains stale views after nested string replacement. Inherited NULLs skip nested payload checks and rejected elements filter their original outer row.
• Compatibility: No wire or storage format changes. DecimalV2 keeps historical execution precision/scale (27, 9); enforcing declared schema parameters and resolving map-key collisions after string truncation remain outside this change. This refactoring targets master without release pick requests.
• Performance impact: Read-only scalar columns and unchanged leaf buffers remain shared, including offsets and null maps. Complex types allocate new shells even when their children are unchanged; this trades small container allocations for simple ownership without recursive detachment. UTs verify storage reuse and source isolation. No throughput benchmark was run.
• Rollback: Revert this commit; no metadata or data migration is needed.

Testing

• BUILD_TYPE_UT=ASAN sh run-be-ut.sh --run --filter='TabletColumnValidatorTest.*' -j48: 24 tests passed on the development machine. Covers shared read-only leaves, nested shell construction, replacement/source isolation, map deduplication, NULL propagation and decimal compatibility.
• Repository clang-format 16 scripts, build hygiene, git diff --check, and prescribed clang-tidy on the changed source TUs and validator UT: passed.
• No standalone BE build or end-to-end regression was rerun for this revision. The ASAN UT script compiled the affected production code and test binary; the existing shared-map regression and generated output are retained.

Release note

Fix incorrect results or failures when inserting shared complex columns that require tablet validation.

Check List (For Author)

• Behavior changed: Yes (see Details)
• Does this need documentation: No
• Pre-submission status: Partially verified: focused ASAN UTs and static checks passed; standalone BE build and end-to-end regression not rerun after this change.

AI Disclosure

• Tool: Codex (GPT-6).
• Generated: Code, tests, and this description.
• Human review: The author reviewed the design and an earlier source version; the final changes remain available for review.

[Mryange]

/review

[github-actions]

Static review of all eight changed files at 7775949 found one new P2 performance regression: unconditional COW copies shared input buffers even when validation is read-only. The inline comment contains the concrete load path and fix direction. No existing inline findings were present. No additional user focus was supplied.

Critical checkpoint conclusions:

• Goal and supporting tests: the extraction provides a single validator entry and protects shared column trees before mutation. The new ownership, nested-row and NULL-handling unit cases support the intended fixes by inspection; the converter-created sharing in the P2 finding is not covered by the unique-column fast-path tests.
• Scope and clarity: changes are confined to validation extraction, converter integration, one nullable child-slot accessor and associated tests. Per-type helpers replace the embedded recursive implementation.
• Concurrency and locking: no threads, atomics or locks are introduced. Writers, group-commit local states and tablet hash partitioner clones own separate converter contexts; no new lock-order issue was found.
• Lifecycle and static initialization: recursive COW detaches shared descendants, child restore guards preserve parent structure on failure, and substring/filter temporaries release borrowed owners before subsequent mutation. Context state/filter references remain within their owners' lifetimes. No ownership cycle or cross-TU static initializer was introduced.
• Configuration: none added; existing string limits and insert strictness remain in use.
• Compatibility: no wire, storage or serialization format change. DecimalV2 retains historical execution precision/scale; enforcing original schema parameters and resolving key collisions after truncation remain preexisting limitations. No new libc API requirement is introduced by the inspected source; no ELF artifact audit was performed.
• Thrift/protobuf definitions: no changes, so field-ID and enum upgrade checks are inapplicable.
• Parallel paths: both tablet writers, group commit and tablet-sink hash partitioning reach the shared converter. Their validation errors propagate before routing, enqueueing or writing affected batches.
• Conditional checks and physical types: inherited NULL exclusion and NOT NULL/BITMAP rejection have concrete scenarios. Top-level constants are materialized before validation; constructors prohibit nested constants. Each map deduplicates before rebuilding its child-to-outer-row mapping, and inherited masks remain aligned through nested arrays/maps/structs.
• Test coverage: all 22 added validator tests and the shared-map insertion regression were inspected. Ownership isolation, strict/non-strict strings, empty containers, nested mapping/NULLs and DecimalV3 widths have representative coverage. The regression orders results, drops tables before use, retains them afterward and uses fixed names. Existing CMake globs include the new source and unit test.
• Expected results: the regression's short/full pairs abc/abcdef and uvw/uvwxyz agree with the SQL inputs and intended truncation. Generated-output provenance and execution success were not independently established in this static review.
• Observability: column/type error prefixes, rejection details, filtered-row counts and validation timing are retained; no new asynchronous component requires additional telemetry.
• Transactions and persistence: no EditLog, partition-version, rowset, delete-bitmap or recovery protocol changes. No new committed/uncommitted visibility behavior was identified.
• Data writes, atomicity and crashes: validation remains before tablet writes. Non-OK status returns stop forwarding the batch; failed temporary output blocks are disposed while input owners survive. No new write atomicity or crash cleanup issue was found by inspection.
• FE/BE variables: none added or changed.
• Performance and memory: the inline P2 identifies an avoidable full-buffer allocation/copy on ordinary shared load columns, including primitive no-op validation. Scoped nested mappings and masks have linear lifetimes; no new resource leak was found. No throughput or memory benchmark was executed.
• Other concerns and review limits: ownership, replacement/error unwinding and nested filtering risks were rechecked with upstream and downstream callers; no other verified issue or unresolved static coverage question remains. No builds, tests, regression runs, runtime validation or source edits were performed in this invocation.

Review converged in two rounds after all assigned reviewers returned NO_NEW_VALUABLE_FINDINGS against the final finding set.

Review converged after 2 round(s).

[github-actions]

[P2] Avoid detaching columns that validation never mutates

OlapTableBlockConvertor first copies the input ColumnPtrs into its output block, so ordinary stream-load columns are still shared when they reach this unconditional IColumn::mutate(). For an INT/BIGINT/DOUBLE column this clones the entire value buffer, only for validate_column() to immediately return OK through its default branch; JSONB and DecimalV3 also only read their buffers. This adds a full-block copy and keeps both copies live for every normal load, whereas these paths previously validated the shared columns directly. Preserve shared ownership for read-only validation and detach only the columns that will actually be modified.

[Mryange]

/review

[github-actions]

Static review of PR #70562 at 4ea20ab found no new actionable issues. Review converged after one round: both complete reviewers and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS; the main review independently verified their conclusions, all seven changed paths and all risk dispositions.

The existing P2 thread 4561445833 is addressed on this head: unconditional scalar detachment is gone, and primitive/JSONB/DecimalV3 validation reuses input ownership. No existing P0/P1 finding remains to block this review. No additional user focus was supplied.

Critical checkpoint conclusions:

• Goal and evidence: immutable recursive validation returns replacement leaves and fresh complex shells, preserving shared inputs. The 24 ownership/validation UTs and shared-map INSERT regression support the intended behavior by inspection; execution was not verified here.
• Scope and reuse: the extraction is focused on tablet validation and reuses core constructors, map deduplication, substring and RuntimeState error reporting.
• Concurrency and locks: no new threads, locks or atomics. Writers, hash partitioner and group-commit local state own separate converters. Shared column data follows the COW contract; no new lock order was introduced.
• Lifecycle: the context references the preceding converter-owned filter map; RuntimeState is refreshed before each call. Row/null-map views stay alive through synchronous recursion and are not retained in results. No reference cycle or cross-TU static initializer was added.
• Configuration: no new settings; existing string soft limits and insert-strict behavior are retained.
• Compatibility: no storage/wire format or distributed protocol change. DecimalV2 retains historical execution precision/scale (27,9). No new libc API or dependency is introduced; release ELF compatibility was not audited.
• Thrift/protobuf: no gensrc changes; field-ID checks are inapplicable.
• Parallel paths: V1/V2 tablet writers and tablet hash partitioning use the same VRowDistribution/converter path; group commit calls the converter directly and filters rejected rows before enqueueing.
• Conditions and NULL/const handling: comments explain byte-versus-character truncation and inherited NULL suppression. Mapping is built after map deduplication, nullable masks combine by ancestry, and failures map to original outer rows. Top-level constants materialize before validation; core constructors exclude nested constants.
• Test coverage: inspected cases cover shared and unique leaves, nested shell/replacement isolation, map duplicate removal, rebuilt row mappings, inherited NULLs/NOT NULL failures, empty nested payloads, legacy decimals and all DecimalV3 widths. The end-to-end regression covers alias isolation under non-strict string truncation.
• Expected results: regression rows correctly retain abc/abcdef and uvw/uvwxyz in the short/full map destinations. Results are ordered and tables are dropped before creation. Output-generation provenance was not rerun.
• Observability: existing column/type error prefixes, filtered-row accounting and validation timers are preserved; no additional telemetry is necessary for this local extraction.
• Transactions and persistence: no EditLog, version, delete-bitmap, commit or failover protocol change.
• Write atomicity and failures: errors propagate upstream and rejected outer rows are excluded before writes. Fresh shells and COW filtering preserve source data on failure; RAII restores recursive diagnostic state. No new teardown or crash-recovery protocol is introduced.
• FE-BE state: no new variables or send paths.
• Performance and memory: read-only leaves/null maps/offsets remain shared; changed strings and duplicate-map filtering allocate replacement storage as needed. Fresh container shells and row/null-map buffers have scoped ownership. No new redundant full-buffer copy was identified; throughput was not measured.
• Other module checks: CMake recursive globs include the new source and UT. The test includes a header, so no source-inclusion unity exemption is required. No PCH, gate-table or explicit-instantiation edits; error Status/Result handling and the public Doris exception boundary remain intact.

Static scope is complete with no unresolved candidates. Builds, tests, sanitizers, style/hygiene tools, benchmarks and runtime checks were not run under this review contract; author-reported results were not treated as locally executed validation.

Review converged after 1 round(s).

[Mryange]

run buildall