[fix](zone-map) Avoid per-row Field temporaries in TypedZoneMapIndexWriter

[csun5285]

Problem

When writing nullable columns, append_nullable splits each block at null boundaries into small non-null runs.

Input block	add_values calls
[1, 3, NULL, NULL, 4, 5, 6, NULL]	add_values([1,3], count=2) + add_values([4,5,6], count=3)

Each add_values call built two heap-allocated Field temporaries just to compare
min/max — allocation, memcpy, variant dispatch, destructor — per call.

Fix

Track page min/max as raw ValType (the same native type std::minmax_element already uses). Materialize Field only once at flush() time.

Benchmark (Release, 65536 rows/iter)

block_rows = post-null-split run length.

Type	block	Baseline (M items/s)	Optimized (M items/s)	Speedup
Int32	1	21.9	240.3	11.0×
Int32	4	75.9	353.0	4.7×
Int32	16	170.0	400.1	2.4×
Int32	64	261.9	394.4	1.5×
Int32	256	314.0	419.7	1.3×
Int32	1024	320.6	425.9	1.3×
Int32	4096	334.2	359.3	1.1×
Double	1	16.7	181.9	10.9×
Double	4	47.0	168.0	3.6×
Double	16	120.0	310.0	2.6×
Double	64	223.1	425.3	1.9×
Double	256	315.1	469.8	1.5×
Double	1024	358.9	477.1	1.3×
Double	4096	373.8	483.3	1.3×
String	1	8.0	59.2	7.4×
String	4	21.7	57.9	2.7×
String	16	41.3	60.7	1.5×
String	64	62.9	80.6	1.3×
String	256	75.2	82.0	1.1×
String	1024	79.4	84.0	1.1×
String	4096	80.4	83.4	1.0×

Small batches (1–16): 2×–11×. Large batches: no regression.

When the columns in the variant are sparse, during compaction, the frequent add calls on the zonemap index cause field creation and field comparison to account for around 12% of CPU usage. After optimization, this drops to 3%.

Release note

None

Check List (For Author)

• Test

  • Regression test
  • Unit Test
  • Manual test (add detailed scripts or steps below)
  • No need to test or manual test. Explain why:
    • This is a refactor/code format and no logic has been changed.
    • Previous test can cover this change.
    • No code files have been changed.
    • Other reason

• Behavior changed:

  • No.
  • Yes.

• Does this need documentation?

  • No.
  • Yes.

Check List (For Reviewer who merge this PR)

• Confirm the release note
• Confirm test cases
• Confirm document
• Add branch pick label

[hello-stephen]

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?

[csun5285]

/review

[csun5285]

run buildall

[github-actions]

Findings

1. be/src/storage/index/zone_map/zone_map_index.cpp: the new string min/max path now copies and compares the full StringRef in the hot path. Because _materialize_page_minmax() immediately goes through Field::create_field_from_olap_value(), bytes after MAX_ZONE_MAP_INDEX_SIZE are still truncated before any page/segment zone map is persisted. That means long STRING/VARCHAR values now pay O(full_value_size) copy/compare cost and keep those full copies alive until page flush without improving the serialized zone map, so this can regress the long-string / variant workloads the patch is trying to speed up.
2. be/benchmark/benchmark_zone_map_index.hpp: make_writer() leaks the StorageField returned by StorageFieldFactory::create(*col) on every benchmark iteration. Since each benchmark loop constructs a fresh writer, the benchmark process accumulates leaked fields, which can skew timings or OOM long runs.

Critical Checkpoint Conclusions

• Goal of the task: reduce TypedZoneMapIndexWriter::add_values() overhead. The numeric path improves that, but the new string path still has a blocking hot-path regression for long values, so the current patch does not fully accomplish the goal yet.
• Scope/minimality: the production change is focused, and the benchmark addition is focused, but the benchmark currently has a resource-management bug.
• Concurrency: no new cross-thread behavior is introduced here; writer state stays local to a single writer instance.
• Lifecycle/static init: page-lifetime state (_page_min, _page_max, _arena) is self-contained inside the writer. I did not find a new lifecycle or static-initialization bug.
• Config/compatibility: no new config, protocol, or storage-format compatibility concerns found.
• Parallel code paths: I did not find another equivalent zone-map write path that also needed the same change.
• Special conditions/comments: the new string-specific fast path is documented, but the current implementation keeps more data than the persisted zone map can ever use.
• Tests: existing be/test/storage/segment/zone_map_index_test.cpp covers basic zone-map serialization, truncation, and float flag behavior, but no new test guards the long-string deferred-materialization path added here.
• Observability: not applicable for this refactor.
• Transactions/persistence/data correctness: I did not find a data-correctness regression in the currently covered paths.
• Performance: blocked by the long-string full-copy/full-compare behavior above.
• Other issues: the benchmark leak makes the evidence in the PR body unreliable until it is fixed.

[csun5285]

/review

[github-actions]

No blocking findings in the modified production path.

Review checkpoints:

• Goal/task: The change achieves its goal of moving page min/max tracking to raw ValType and materializing Field only at flush() time. Existing be/test/storage/segment/zone_map_index_test.cpp already exercises repeated add_values()/flush() behavior across int, string, float special values, decimal, and date/time types, although there is no new targeted test for this exact hot path.
• Scope/focus: The production change is small and localized to TypedZoneMapIndexWriter, with the benchmark kept separate.
• Concurrency: No new concurrent path is introduced. This writer state is still page-local and used through the existing single-writer column-write call chain, so there are no new lock ordering or thread-safety concerns here.
• Lifecycle: The new _page_min, _page_max, _page_has_minmax, and _arena state is reset on every flush(). For string types, bytes are copied into _arena, then materialized into Field before _arena.clear(), so I do not see a dangling-reference issue escaping the writer.
• Config/compatibility: No new config, FE/BE protocol, or on-disk format change is introduced. ZoneMapPB serialization remains on the existing path.
• Parallel code paths: The optimization is applied in the shared zone-map writer path used by the nullable-run split workload described in the PR, so I did not find a sibling production path in this abstraction that also needed the same change.
• Special checks: String truncation behavior and float NaN/+Inf/-Inf flag handling remain aligned with the pre-change logic.
• Observability/persistence/transactions/data writes: This change only affects in-memory min/max accumulation before the existing serialization path; I do not see new observability, persistence, transactionality, or data-visibility risks.
• Performance: The direction is sound. The hot path removes repeated Field construction while keeping materialization at page flush.

Residual risk: I reviewed the code and existing unit coverage, but I did not run a BE build or tests in this runner, so the remaining risk is limited to unexecuted build/benchmark compilation rather than a production-path correctness concern.

[csun5285]

run buildall

[csun5285]

run buildall

[csun5285]

/review

[github-actions]

No blocking issues found in this PR.

Critical checkpoints:

• Goal / correctness: The change achieves its stated goal of removing per-call Field temporary construction from TypedZoneMapIndexWriter while preserving the existing zone-map serialization path. The existing unit tests in be/test/storage/segment/zone_map_index_test.cpp already exercise repeated add_values() calls, string truncation, pass-all pages, and multiple primitive/date/time types against the same writer logic.
• Change scope: The modification is small and focused. It only adjusts the zone-map writer hot path and adds a benchmark wired into the benchmark binary.
• Concurrency: No new concurrency is introduced. TypedZoneMapIndexWriter remains object-local writer state, and the PR does not change locking, thread interaction, or lifecycle ordering.
• Lifecycle / initialization: No special lifecycle or static-initialization risk was introduced. The new _page_min, _page_max, and _page_has_minmax state is reset during flush().
• Configuration: No new config item was added.
• Compatibility: No FE/BE protocol change or on-disk format change was introduced. ZoneMapPB serialization/deserialization remains unchanged.
• Parallel paths: I did not find another equivalent zone-map writer path that also needed the same change.
• Special conditions: The string truncation behavior and float/double special-value handling follow the existing semantics of this module.
• Test coverage: Existing unit coverage is relevant for this change, but no new automated test was added specifically for the deferred materialization path, and I did not run BE tests in this runner.
• Observability: No additional logs or metrics are needed for this localized hot-path optimization.
• Transaction / persistence / data writes: This touches the storage write path, but it does not alter transaction semantics or persisted metadata shape, so I do not see a replay/compatibility concern from this diff.
• Performance: The optimization removes hot-path Field materialization and defers it to flush(). The added benchmark is aligned with the intended compaction/write workload, and I did not identify an obvious performance regression in the changed logic.
• Other issues: None blocking found.

Residual risk:

• I relied on static review plus the existing zone-map unit coverage; I did not build or run tests locally in this runner.

[hello-stephen]

BE UT Coverage Report

Increment line coverage 100.00% (45/45) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	53.12% (20193/38013)
Line Coverage	36.68% (190143/518336)
Region Coverage	32.92% (147554/448233)
Branch Coverage	34.06% (64610/189702)

[hello-stephen]

BE Regression && UT Coverage Report

Increment line coverage 100.00% (45/45) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	73.61% (27400/37222)
Line Coverage	57.30% (296095/516718)
Region Coverage	54.27% (245503/452343)
Branch Coverage	56.07% (106693/190270)

[csun5285]

run buildall

[hello-stephen]

BE UT Coverage Report

Increment line coverage 100.00% (45/45) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	53.22% (20329/38199)
Line Coverage	36.77% (191357/520384)
Region Coverage	33.10% (148850/449654)
Branch Coverage	34.17% (65004/190236)

[hello-stephen]

BE Regression && UT Coverage Report

Increment line coverage 100.00% (45/45) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	73.85% (27629/37412)
Line Coverage	57.61% (298905/518804)
Region Coverage	54.95% (249365/453837)
Branch Coverage	56.46% (107743/190826)

[csun5285]

/review

[github-actions]

No blocking findings after reviewing be/src/storage/index/zone_map/zone_map_index.{h,cpp} and the new benchmark wiring.

Critical checkpoint conclusions:

• Goal / correctness: The PR goal is to remove per-call Field materialization from TypedZoneMapIndexWriter::add_values(). The new _page_min/_page_max cache does that while keeping page and segment zone-map serialization behavior unchanged. Existing zone_map_index_test coverage for string truncation, CHAR padding, pass_all, and float/double special values still exercises the touched semantics.
• Scope / minimality: The functional change is tightly scoped to the zone-map writer, and the benchmark is isolated under be/benchmark/.
• Concurrency: No new concurrency is introduced. TypedZoneMapIndexWriter remains page-local mutable state reset at flush().
• Lifecycle: The new raw min/max state is page-scoped and cleared together with _page_zone_map after each flush. For string types, the cached StringRef storage is backed by _arena and cleared only after materialization/serialization completes.
• Config / compatibility / FE-BE protocol: No new config, no storage-format change, no FE-BE contract change, and no parallel code path appears to be missed because all supported primitive writers still flow through ZoneMapIndexWriter::create().
• Special conditions: pass_all, truncation to MAX_ZONE_MAP_INDEX_SIZE, and the max-value byte bump remain in the same logical order as before, so pruning semantics are preserved.
• Testing: No new unit/regression test was added, but the existing BE unit tests already cover the semantics touched here. Residual risk is limited to the performance-oriented split-run call pattern that the new benchmark models rather than an observed correctness gap.
• Observability / transactions / persistence: No new observability requirements, and the zone-map protobuf format is unchanged.
• Performance: The hot path removes repeated Field construction/destruction with no obvious correctness regression from code inspection.

Overall opinion: the change is small, focused, and looks correct.

[csun5285]

run buildall

[hello-stephen]

BE UT Coverage Report

Increment line coverage 100.00% (45/45) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	53.34% (20371/38194)
Line Coverage	36.88% (191941/520462)
Region Coverage	33.19% (149273/449723)
Branch Coverage	34.29% (65267/190346)

[csun5285]

/review

[github-actions]

I reviewed the full PR, including the existing inline review threads and the surrounding write/read paths. I did not repeat the two already-raised issues (the benchmark leak/unchecked create status, and the earlier full-string-copy regression), but they remain part of the open review context.

Additional findings from this pass:

• be/src/storage/index/zone_map/zone_map_index.h: the new arena-backed string min/max path adds retained per-page allocations that are invisible to ZoneMapIndexWriter::size(), so ScalarColumnWriter::estimate_buffer_size() / SegmentWriter::estimate_segment_size() under-account memory.
• be/benchmark/benchmark_zone_map_index.hpp: the added string benchmark only uses 16-byte strings, so it does not measure the > MAX_ZONE_MAP_INDEX_SIZE truncation/copy path that this PR changes.

Critical checkpoints:

• Goal / proof: the PR aims to remove per-call Field temporaries from TypedZoneMapIndexWriter::add_values(). The numeric fast path does that, but there is no new test covering the new raw-ValType / arena lifecycle.
• Scope: focused to the zone-map writer and its benchmark.
• Concurrency: no new concurrent path; the writer is still used from the column-writer thread without new locking.
• Lifecycle: page/segment field materialization is safe across flush(), but the new arena-backed string state is not fully tracked in memory accounting.
• Configs: none.
• Compatibility / storage format: unchanged; no FE/BE protocol change.
• Parallel paths: I did not find another functional zone-map write path that needed the same refactor.
• Special conditional checks: existing pass_all and string max-byte bump logic remain unchanged.
• Test coverage: existing BE zone-map tests cover serialized truncation semantics, but no new test covers this refactor's memory/accounting behavior or the claimed long-string benchmark scenario.
• Test result updates: none.
• Observability: no new logging/metrics needed.
• Transactions / persistence / data writes: no new transaction or persistence risks found; the on-disk zone-map format remains the same.
• FE/BE variable passing: none involved.
• Performance: the numeric hot path should improve, but the string benchmark evidence is incomplete and the new string path introduces extra resident memory that current accounting misses.
• Other issues: see inline comments.

User focus points:

• No additional user-provided review focus.

[csun5285]

run buildall

[hello-stephen]

BE UT Coverage Report

Increment line coverage 100.00% (45/45) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	53.34% (20416/38273)
Line Coverage	36.90% (192451/521553)
Region Coverage	33.21% (149686/450757)
Branch Coverage	34.33% (65496/190768)

[csun5285]

/review

[hello-stephen]

BE Regression && UT Coverage Report

Increment line coverage 100.00% (45/45) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	72.06% (27020/37498)
Line Coverage	55.28% (287574/520258)
Region Coverage	52.45% (238678/455083)
Branch Coverage	53.71% (102861/191498)

[github-actions]

I completed a full review of the PR, read the existing inline threads first, and did not find any additional distinct blocking issues beyond the concerns already raised there. Those existing findings are still sufficient to block merge:

1. The new benchmark still ignores Status returns from ZoneMapIndexWriter::create() / flush(), which violates Doris error-handling rules and can silently produce invalid benchmark results.
2. The benchmark does not exercise >512-byte strings, so the PR body's string-performance claims are not supported by the added benchmark coverage.
3. The new string min/max path retains arena-backed memory that is not reported by ZoneMapIndexWriter::size(), so ScalarColumnWriter::estimate_buffer_size() / SegmentWriter::estimate_segment_size() under-account memory. Because Arena::clear() keeps the head chunk for reuse, a single large page can also pin that chunk for the rest of the segment writer lifetime while the estimate remains flat.

Critical checkpoints:

• Goal / correctness: The goal is to remove per-call Field temporaries in TypedZoneMapIndexWriter::add_values(). The deferred-materialization approach appears functionally correct for page/segment min/max semantics, and existing unit tests already cover truncation / pass_all behavior. However, the PR is not ready because the remaining benchmark and memory-accounting issues undermine the claimed benefit and violate Doris runtime accounting expectations.
• Scope / focus: The code change is locally focused in the zone-map writer plus a benchmark, but the benchmark and accounting follow-through are still incomplete.
• Concurrency: I did not find new concurrency or lock-order issues in this path; the writer remains single-owner / single-thread in the existing call chain.
• Lifecycle: The new string path introduces arena-backed lifetime for page min/max. That lifecycle is internally consistent, but its memory is not surfaced through the existing size-accounting interface.
• Config / compatibility: No new config or storage-format compatibility issue found.
• Parallel paths: I did not find another equivalent writer path that also needed the same optimization.
• Special conditions: The truncation / max-byte increment logic remains aligned with the prior zone-map semantics.
• Test coverage: Existing direct unit tests cover zone-map serialization semantics, including long-string truncation and pass_all, but the PR adds no test for the new deferred-materialization hot path itself and no caller-level coverage for the nullable-run / ScalarColumnWriter scenario described in the PR body.
• Observability: No new observability requirement found.
• Transactions / persistence / writes: No FE transaction or metadata-log impact found; this is a BE in-memory write-path optimization only.
• Performance: The optimization should help the intended hot path, but the current benchmark evidence is incomplete for long strings and still contains ignored-error handling.

User focus: review_focus.txt contained no additional user-provided focus points.

Please address the existing inline findings before merge.

[csun5285]

1. The purpose of this PR is to optimize small-batch writes, not large strings of 512+ bytes. For large strings, the previous semantics are preserved.
2. ZoneMapIndexWriter::size() only accounts for the serialized ZoneMapPB size; let's keep the previous semantics for now.

[csun5285]

run buildall

[eldenmoon]

run buildall

[csun5285]

run buildall

[yiguolei]

run buildall

[yiguolei]

/review

[github-actions]

PR approved by at least one committer and no changes requested.

[github-actions]

Code review completed for PR 62544. I reviewed the existing inline threads first and did not duplicate those already-known concerns.

Summary opinion: no additional non-duplicative blocking issue found in the current diff. The implementation is focused on avoiding per-call Field materialization in TypedZoneMapIndexWriter, with the string truncation moved into the zone-map hot path and a segment merge guard for all-null pages.

Critical checkpoint conclusions:

• Goal/test coverage: The PR goal is achieved by tracking page min/max as raw ValType and materializing Field at flush; new unit tests cover the all-null-page segment max regression and existing zone-map tests cover string truncation/materialization behavior.
• Scope/focus: The change is small and localized to zone-map writer behavior, its Field helper dependency, benchmark coverage, and BE unit tests.
• Concurrency/lifecycle: No new shared concurrency or lock path is introduced. The page min/max lifetime is local to the writer; string refs borrow from per-writer std::string storage and are materialized before serialization.
• Configuration/compatibility: No new config or persistent format change is introduced; serialized ZoneMapPB semantics remain compatible.
• Parallel paths: The touched path is the ScalarColumnWriter zone-map path; no separate BE/Cloud path appeared to require the same logic.
• Data correctness: The new guard avoids merging all-null page stale min/max into the segment zone map. Existing review threads already discuss the earlier min/max/truncation concerns, so I did not repeat them.
• Memory/performance: The hot path removes Field temporaries and replaces arena-backed retained copies with bounded std::string storage for current page min/max. Existing benchmark-status-check/long-string coverage comments are already known from prior threads.
• Observability: No new logging/metrics seem necessary for this local write-path optimization.
• User focus: No additional user-provided review focus was specified.

Verification: attempted ./run-be-ut.sh --run --filter=ColumnZoneMapTest.*, but it timed out during third-party submodule initialization before the focused tests could execute.

it is ok

[yiguolei]

skip check_coverage