perf(cpp): fill repeated TAG values in bulk by ColinLeeo · Pull Request #1088 · apache/tsfile

ColinLeeo · 2026-11-12T08:19:12Z

Problem

A block's TAG (id) column repeats the same value on every row, but ColAppender::fill() appended it row by row — one add_row() plus one append() per row. On a table scan that reads the TAG column this showed up as ~10% of read time in a sampled profile (fill_ids -> ColAppender::fill -> append).

Change

Add Vector::fill_repeated(value, len, count), defaulting to the existing per-row loop, overridden by FixedLengthVector and VariableLengthVector.
Both overrides go through new ByteBuffer::fill_repeated_variable() / fill_repeated_fixed() helpers: write the value pattern once, then double the filled region, so the per-row work becomes one memcpy step instead of a full append.
ColAppender::fill() now calls fill_repeated() and keeps the same capacity/error checks.

Benchmark

3.2M-row full scan, single thread:

	read
before	0.297–0.299 s
after	0.267–0.272 s

~10% faster with identical output.

Testing

Full C++ suite: 1454 tests passed.

A block's TAG (id) column repeats the same value on every row, but ColAppender::fill() appended it row by row: one add_row() plus one append() per row. On a 3.2M-row table scan reading the TAG column this showed up as ~10% of read time in a sampled profile. Add Vector::fill_repeated() (defaulting to a per-row loop) with overrides for the fixed-length and variable-length vectors. Both build the value pattern once and then double the filled region, so the per-row work becomes a single memcpy copy step instead of a full append. 3.2M-row full scan: 0.298s -> 0.269s (~10%) with unchanged output.

Exercise the fixed-length and variable-length overrides directly: value and length correctness after multiple fills, row counters, capacity overflow rejection, and the zero-count no-op.

codecov-commenter · 2026-11-12T09:21:31Z

Codecov Report

❌ Patch coverage is 17.30769% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.50%. Comparing base (90c7862) to head (e9e9a23).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines	Patch %	Lines
cpp/src/common/container/byte_buffer.h	0.00%	32 Missing ⚠️
cpp/src/common/tsblock/vector/vector.h	0.00%	4 Missing ⚠️
...pp/src/common/tsblock/vector/fixed_length_vector.h	40.00%	1 Missing and 2 partials ⚠️
...src/common/tsblock/vector/variable_length_vector.h	40.00%	2 Missing and 1 partial ⚠️
cpp/src/common/tsblock/tsblock.h	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1088      +/-   ##
===========================================
- Coverage    66.51%   66.50%   -0.01%     
===========================================
  Files          778      782       +4     
  Lines        56577    56761     +184     
  Branches      9105     9141      +36     
===========================================
+ Hits         37633    37751     +118     
- Misses       17291    17344      +53     
- Partials      1653     1666      +13

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Review follow-ups on the new fill path: * Both ByteBuffer fill helpers wrote the first element before looking at count, so a count == 0 call wrote into the slack after the data — or past the end of an exactly-full buffer. Return early instead. * FixedLengthVector::fill_repeated used the caller's len as the element stride while every read path advances by type_len_. Use type_len_ and assert the caller agrees. * ColAppender::fill reported every failure (including E_OOM from a failed growth) as E_INVALID_ARG; propagate the actual code. Tests: zero-count at the ByteBuffer level (fails without the guard) and a variable-length fill larger than the per-row reservation, which exercises the grow-and-copy path.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

3 open issues

ByteBuffer now uses std::memcpy, but this header includes <string.h> rather than <cstring>.… · New
Relying on ASSERT(len == type_len_) is unsafe in release builds if ASSERT compiles out:… · New
This test reads an int32_t via reinterpret_cast<int32_t*>(read) from a char* buffer. If the… · New

What changed in this PR

This PR optimizes filling repeated TAG/id column values by adding bulk “fill repeated” APIs at the Vector/ByteBuffer layers and updating ColAppender to use them, reducing per-row append overhead.

Changes:

Added Vector::fill_repeated() with specialized overrides in fixed/variable-length vectors.
Added ByteBuffer::fill_repeated_variable() / fill_repeated_fixed() using a doubling memcpy strategy.
Updated ColAppender::fill() to use bulk fill and added targeted regression tests.

File	Description
cpp/test/common/tsblock/tslock_test.cc	Adds tests covering repeated fill for fixed/variable vectors, overflow handling, grow path, and zero-count no-op.
cpp/src/common/tsblock/vector/vector.h	Introduces `Vector::fill_repeated()` default implementation.
cpp/src/common/tsblock/vector/variable_length_vector.h	Overrides `fill_repeated()` to use `ByteBuffer::fill_repeated_variable()`.
cpp/src/common/tsblock/vector/fixed_length_vector.h	Overrides `fill_repeated()` to use `ByteBuffer::fill_repeated_fixed()`.
cpp/src/common/tsblock/tsblock.h	Switches `ColAppender::fill()` to call `vec_->fill_repeated()` and handles row count/capacity checks.
cpp/src/common/container/byte_buffer.h	Adds bulk repeated-fill helpers implementing doubling-copy strategy.

_{Review effort: Lite}

Return E_INVALID_ARG when the caller's len differs from type_len_ instead of relying on ASSERT, which is compiled out in release and would make the fill read past a shorter value buffer. Also include <cstring> for std::memcpy and copy test values into aligned locals instead of reinterpret_cast'ing the row buffer.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

5 open issues

std::memcpy(dst, &len, variable_type_len_) assumes variable_type_len_ <= sizeof(len); otherwise… · New
std::memcpy(dst, &len, variable_type_len_) assumes variable_type_len_ <= sizeof(len); otherwise… · New
reinterpret_cast<int32_t*>(read) can be undefined behavior on platforms that require alignment… · New
The newly added <cstring> include does not appear to be used in the shown changes. If it’s not… · New
This initializes only the first 3 bytes and copies all 16 bytes into the buffer. While not… · New

3 resolved since last review

_{Review effort: Lite}

Return E_INVALID_ARG if variable_type_len_ ever exceeds sizeof(uint32_t) before the length prefix is copied from a uint32_t (it is fixed to sizeof(uint32_t) today, but make the invariant explicit). Also copy the int32_t test value out with memcpy instead of reinterpret_cast'ing the row buffer, which is what <cstring> was added for.

Same isolation fix as the writer tests: parallel CTest processes can seed the name generator in the same clock tick, and one then removes the file another is opening (E_ALREADY_EXIST at the fixture's open, observed on the macOS 3.11 and py-msvc 3.11 jobs). Append pid and a per-process counter.

ColinLeeo added 3 commits November 12, 2026 16:02

style: apply spotless formatting

a0ec911

test(cpp): cover Vector::fill_repeated bulk TAG filling

1f2916b

Exercise the fixed-length and variable-length overrides directly: value and length correctness after multiple fills, row counters, capacity overflow rejection, and the zero-count no-op.

ColinLeeo requested a balanced review from Copilot November 12, 2026 15:01

Copilot AI reviewed Nov 12, 2026

View reviewed changes

Comment thread cpp/src/common/container/byte_buffer.h Resolved

Comment thread cpp/src/common/tsblock/vector/fixed_length_vector.h Resolved

Comment thread cpp/test/common/tsblock/tslock_test.cc Resolved

ColinLeeo requested a balanced review from Copilot November 12, 2026 16:40

Copilot AI reviewed Nov 12, 2026

View reviewed changes

ColinLeeo added 2 commits November 13, 2026 01:04

Copilot stopped reviewing on behalf of ColinLeeo due to an error November 12, 2026 18:00
The job was not acquired by a runner even after multiple attempts.

Copilot stopped reviewing on behalf of ColinLeeo due to an error November 12, 2026 18:50
The job was not acquired by a runner even after multiple attempts.

Navigation Menu

Repository navigation

perf(cpp): fill repeated TAG values in bulk - #1088

perf(cpp): fill repeated TAG values in bulk#1088
ColinLeeo wants to merge 7 commits into
developfrom
perf/tag-column-bulk-fill

ColinLeeo commented Nov 12, 2026

Uh oh!

codecov-commenter commented Nov 12, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ColinLeeo commented Nov 12, 2026

Problem

Change

Benchmark

Testing

Uh oh!

codecov-commenter commented Nov 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Nov 12, 2026 •

edited

Loading