Repository navigation
Conversation
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 Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1088 +/- ##
===========================================
- Coverage 66.94% 66.89% -0.06%
===========================================
Files 788 788
Lines 57573 57630 +57
Branches 9304 9310 +6
===========================================
+ Hits 38542 38551 +9
- Misses 17355 17400 +45
- Partials 1676 1679 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Contributor
There was a problem hiding this comment.
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
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 doublingmemcpystrategy. - 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.
Contributor
There was a problem hiding this comment.
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_)assumesvariable_type_len_ <= sizeof(len); otherwise… · Newstd::memcpy(dst, &len, variable_type_len_)assumesvariable_type_len_ <= sizeof(len); otherwise… · Newreinterpret_cast<int32_t*>(read)can be undefined behavior on platforms that require alignment… · New<cstring>include does not appear to be used in the shown changes. If it’s 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.
Copilot stopped reviewing on behalf of
ColinLeeo due to an error
November 12, 2026 18:00
Copilot stopped reviewing on behalf of
ColinLeeo due to an error
November 12, 2026 18:50
develop added set_data_size() (direct-decode support) where this branch adds the fill_repeated_* helpers; keep both.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A critical size-calculation overflow can bypass capacity and 4 GiB checks, allowing a roughly 4 GiB payload copy into a 64-byte buffer.
1 open issue
5 resolved since last review
std::memcpy(dst, &len, variable_type_len_)assumesvariable_type_len_ <= sizeof(len); otherwise…std::memcpy(dst, &len, variable_type_len_)assumesvariable_type_len_ <= sizeof(len); otherwise…reinterpret_cast<int32_t*>(read)can be undefined behavior on platforms that require alignment…<cstring>include does not appear to be used in the shown changes. If it’s not…
Review effort: Balanced
elem_size * count multiplies two 32-bit values, so with len near UINT32_MAX and a large count the product wraps uint64_t, need lands below the capacity check and a ~4 GiB copy runs into the small buffer. Bound the count by the remaining 32-bit capacity first and return E_OOM; the fixed-width helper gets the same check. Test reproduces the wrapping case (crashes without the guard).
Same parallel-CTest collision as the reader tests: two processes can seed from the same clock tick and one removes the file the other opens (E_FILE_OPEN_ERR), observed on the macOS Release job.
Several fixtures build their .tsfile name from a clock count alone. CTest runs cases in parallel processes, so two can read the same tick and then one removes the file the other is opening or reopening (E_ALREADY_EXIST / E_FILE_OPEN_ERR, seen as SEGFAULT on macOS). Append the process id and a per-process counter, the pattern the writer tests already use. Covers table_read_failure, table_field_filter, chunk_reader_resource, aligned_chunk_reader_bulk, aligned_field_filter_regression, masked_bulk_copy and timeseries_schema_codec.
… perf/tag-column-bulk-fill # Conflicts: # cpp/test/reader/table_field_filter_test.cc
Contributor
There was a problem hiding this comment.
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
6 open issues
available / elem_sizewill divide by zero ifelem_size == 0. Whilevariable_type_len_is… · New#ifdef ENABLE_THREADS, but the file now usesgetpid()/… · Newgetpid()/_getpid()andstd::atomicusage, but the diff places the pid headers… · NewByteBuffer::fill_repeated_variable()can returnE_INVALID_ARG(e.g., invalid… · New
1 resolved since last review
Review effort: Lite
Copilot stopped reviewing on behalf of
ColinLeeo due to an error
November 13, 2026 11:03
getpid()/_getpid() are used in the fixtures unconditionally, but the process.h/unistd.h block landed inside the ENABLE_THREADS guard in aligned_chunk_reader_bulk_test and chunk_reader_resource_test, which would break a build with threads disabled.
… perf/tag-column-bulk-fill
Reject elem_size == 0 before dividing, and let the variable-length vector return the underlying error code (E_INVALID_ARG vs E_OOM) instead of always reporting E_OOM.
Resolve the table field filter test conflict using the unique-file-name changes already merged in develop.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A block's TAG (id) column repeats the same value on every row, but
ColAppender::fill()appended it row by row — oneadd_row()plus oneappend()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
Vector::fill_repeated(value, len, count), defaulting to the existing per-row loop, overridden byFixedLengthVectorandVariableLengthVector.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 callsfill_repeated()and keeps the same capacity/error checks.uint64_toverflow before the capacity checks, and reject a zero element size.Benchmark
3.2M-row full scan, single thread:
~10% faster with identical output.
Testing
Full C++ suite: 1535 tests passed.
The branch also carries test-only changes that make clock-derived
.tsfilenames unique per process (parallel CTest could otherwise collide). They are being landed separately in #1102; once that merges intodevelop, mergingdevelophere collapses them and this PR's diff returns to the fill change plus its tests.