perf(cpp): bulk-copy contiguous boundary pages and decode aligned pages in place by ColinLeeo · Pull Request #1096 · apache/tsfile

ColinLeeo · 2026-11-12T10:30:25Z

Motivation

Two costs remain in AlignedChunkReader when a multi-column aligned chunk is read through the page plan:

Only FULL_PASS pages are bulk-copied. A page that a contiguous time predicate fully covers is still classified BOUNDARY, so every one of its rows is re-checked against the predicate and appended one at a time.
Each retained page is decoded into a per-page staging buffer and then copied into the destination TsBlock.

Changes

Contiguous BOUNDARY pages take the bulk path. Filter::is_contiguous_time_filter() (default false, TimeBetween without negation overrides it to true) says that every timestamp between a page's first and last retained row matches, so [row_begin, row_end) can be copied as a block. Discrete predicates (TimeIn) and negated forms keep the row-by-row path; a FIELD predicate keeps it too, because a page mask can still reject rows inside the retained range.

Leading pages decode straight into the destination block. prepare_direct_decode() decides how many leading pages of the plan can be written into the destination column buffers instead of a staging buffer. This requires a fixed destination address, so it applies only when

no FIELD filter is set (a mask can drop rows and shift the output),
every value column is fixed-length and matches the destination vector type,
the page is retained from row 0 through its end (partial ranges and offset pushdown stop the prefix),
the scatter consumes those pages within the same call — on the eager (parallel) path the whole run that fits in the current block, on the lazy path the whole plan.

ByteBuffer::set_data_size() declares bytes written through get_data(); the scatter then advances the column length instead of copying bytes onto themselves.

Values, null handling and ordering are unchanged: the row-by-row path still handles pages with nulls, partial ranges, variable-length columns and missing measurements.

Tests

New cpp/test/reader/aligned_chunk_reader_bulk_test.cc — 40 rows in 8-row pages (5 pages per chunk), fixed-length and STRING columns, driven through a decode pool so the page-plan path runs (the production parallel-read configuration):

ContiguousWindowBulkCopiesInnerPages — full pages between the window edges are bulk-copied and produce exactly the matching rows.
WindowInsideOnePageIsBulkCopied — a window inside a single page copies only the retained slice.
DiscreteTimeFilterStaysOnSlowPath — TimeIn keeps the row-by-row path (holes inside a page).
NoFilterDecodesDirectlyIntoBlock — no filter: pages are decoded in place (TEST_direct_page_end() > 0).
VariableLengthColumnSkipsDirectDecode — a STRING column disables in-place decode.
RowsSpanMultipleBlocks — rows resume correctly across block swaps.
FieldFilterKeepsPartialPageOnSlowPath — a half-retained page is not bulk-copied.

Full TsFile_Test suite passes; mvn -P with-cpp spotless:check passes.

Benchmark

Scan of 200k rows x 8 INT64 columns, window covering the middle half, 30 iterations, release build, macOS:

pages	block	develop	this PR
64 KiB (8192 points)	2 MB (default)	2.39 ms/scan	2.08 ms/scan (-13%)
64 KiB	4 MB	2.41 ms/scan	2.10 ms/scan (-13%)
4 KiB (512 points)	4 MB	5.12 ms/scan	5.12 ms/scan

With 4 KiB pages the scan is bound by the per-page reads (~3k preads per scan) and the copy work no longer matters; with 64 KiB pages the scatter dominates and the bulk path with in-place decode shows up.

…es in place The aligned chunk reader bulk-copied FULL_PASS pages only. A page can also be fully matched while classified BOUNDARY: under a contiguous time predicate (TimeBetween without negation) every timestamp inside the page's retained [row_begin, row_end] range passes, so the range can be copied as a block instead of re-checking and appending row by row. Discrete predicates (TimeIn) and negated forms keep the row-by-row path, and a FIELD predicate keeps it too: a page mask can still reject rows inside the retained range. Second, on the page-plan path the leading pages whose output rows fit in the current TsBlock are now decoded straight into the destination column buffers instead of a per-page staging buffer that is copied afterwards. That needs a fixed destination address, so it applies only to fixed-length columns, only when a page is retained from row 0 through its end, and only to the prefix that the scatter consumes from the same block; eager (parallel) decode can use it for every page that fits. Pages with nulls, partial ranges and variable-length columns keep their staging buffers. ByteBuffer gains set_data_size() to declare bytes written directly through get_data(). On a scan of 200k rows x 8 INT64 columns with a window covering the middle half, 64 KiB pages and the default block size, a full read takes 2.08 ms instead of 2.39 ms per scan. With 4 KiB pages the read is I/O bound and the change is neutral (5.1 ms both ways).

codecov-commenter · 2026-11-12T10:58:26Z

Codecov Report

❌ Patch coverage is 93.02326% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.59%. Comparing base (90c7862) to head (87cbe30).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines	Patch %	Lines
cpp/src/reader/aligned_chunk_reader.cc	92.59%	2 Missing and 4 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1096      +/-   ##
===========================================
+ Coverage    66.51%   66.59%   +0.08%     
===========================================
  Files          778      782       +4     
  Lines        56577    56785     +208     
  Branches      9105     9154      +49     
===========================================
+ Hits         37633    37817     +184     
- Misses       17291    17301      +10     
- Partials      1653     1667      +14

☔ 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.

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

4 open issues

bulk_page_count_ is incremented per bulk-copy operation, not per distinct page. If a single… · New
TEST_bulk_page_count() appears to be a cumulative counter (\"so far\"), but the test accumulates… · New
This comment reads as generally true, but this file also has a test that uses a FIELD filter… · New
expected doesn't mutate range, so taking const TimeBetween& would better communicate intent… · New

What changed in this PR

Improves AlignedChunkReader page-plan performance by enabling bulk-copy for contiguous BOUNDARY pages and decoding eligible leading pages directly into the destination TsBlock buffers to avoid staging/copy overhead.

Changes:

Add Filter::is_contiguous_time_filter() with TimeBetween marking non-negated ranges as contiguous for bulk-copy eligibility.
Implement direct-decode prefix selection (prepare_direct_decode) and in-place decode plumbing in AlignedChunkReader.
Add new C++ tests covering contiguous-window bulk-copy, direct decode, and cases that must stay on the slow path.

File	Description
cpp/test/reader/aligned_chunk_reader_bulk_test.cc	Adds new tests validating bulk-copy and direct-decode behaviors on the page-plan path.
cpp/src/reader/filter/time_operator.h	Marks `TimeBetween` (non-negated) as a contiguous time filter.
cpp/src/reader/filter/filter.h	Introduces `is_contiguous_time_filter()` API to classify time predicates for bulk-copy.
cpp/src/reader/aligned_chunk_reader.h	Adds direct-decode/bulk-count test hooks and per-page direct buffer aliasing fields.
cpp/src/reader/aligned_chunk_reader.cc	Implements direct-decode prefix selection, in-place decode, and contiguous-BOUNDARY bulk-copy logic.
cpp/src/common/container/byte_buffer.h	Adds `ByteBuffer::set_data_size()` for declaring bytes written via raw buffer access.

_{Review effort: Lite}

TEST_bulk_page_count() counted bulk-copy operations, so a page split across several calls (small destination block) was counted more than once. Count a page when its first bulk operation starts, and have the test read the cumulative counter as a delta instead of adding it up per call. Also scope the skip macro comment to the tests that actually need a decode pool; the FIELD filter test reaches the page plan either way.

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

8 open issues

Direct-decode bookkeeping can become inconsistent when direct_page_end_ includes a page that… · New
Direct-decode bookkeeping can become inconsistent when direct_page_end_ includes a page that… · New
Direct-decode bookkeeping can become inconsistent when direct_page_end_ includes a page that… · New
Direct-decode bookkeeping can become inconsistent when direct_page_end_ includes a page that… · New
TEST_direct_page_end() / TEST_bulk_page_count() are only available under #ifdef ENABLE_TEST… · New
The comment describes the range as [row_begin, row_end], but elsewhere row_end is treated as… · New
bulk_pages is treated as an in/out accumulator (*bulk_pages += ...) but the function… · New
bulk_pages is treated as an in/out accumulator (*bulk_pages += ...) but the function… · New

4 resolved since last review

_{Review effort: Lite}

…mits A page inside the direct-decode prefix that cannot be written in place (it carries nulls, or its retained range is shorter than the page) still has to reserve space in the destination column, because the pages after it were already decoded at offsets computed from it. The reservation used the whole page's non-null count; derive it from the non-null values in [row_begin, row_end) instead, which is exactly what the scatter appends for that page, so the offsets stay right whatever mix of pages the prefix holds. Also fix the comment to use the half-open [row_begin, row_end) range, and let read_all() zero its out parameters so callers need not pre-zero them.

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

6 open issues

Direct decode writes into the destination ByteBuffer via get_data() before the buffer’s… · New
Direct decode writes into the destination ByteBuffer via get_data() before the buffer’s… · New
This test writes a deterministic filename into the current working directory and assumes process… · New
time_meta_ is reassigned for every index even though it represents the aligned time column… · New
These are hard prerequisites for the rest of the test; if either fails, subsequent reads can… · New
These are hard prerequisites for the rest of the test; if either fails, subsequent reads can… · New

8 resolved since last review

_{Review effort: Lite}

Give each case's file a wall-clock suffix so parallel or repeated runs cannot collide on the name, take the shared aligned time chunk from the first index and assert the rest agree instead of silently overwriting it, and stop the read helper on a failed load or block init instead of asserting further against an unusable block.

The CI run for this PR failed in HotDeviceMetadataTest.RepeatedLookups... with the fixture's open() returning E_ALREADY_EXIST, and the test body then segfaulted on the unopened writer. The name is seeded from the wall clock, and cases of one suite run as separate processes in a shared directory (ctest --parallel), so two of them can land on the same seed and race on open() with O_EXCL. Seed from random_device instead, and assert the open so a failure is reported rather than crashing the body.

ColinLeeo requested a balanced review from Copilot November 12, 2026 14:17

Copilot AI reviewed Nov 12, 2026

View reviewed changes

Comment thread cpp/src/reader/aligned_chunk_reader.cc Resolved

Comment thread cpp/test/reader/aligned_chunk_reader_bulk_test.cc Resolved

Comment thread cpp/test/reader/aligned_chunk_reader_bulk_test.cc Resolved

Comment thread cpp/test/reader/aligned_chunk_reader_bulk_test.cc Resolved

ColinLeeo requested a lite review from Copilot November 12, 2026 14:55

Copilot AI reviewed Nov 12, 2026

View reviewed changes

ColinLeeo requested a lite review from Copilot November 12, 2026 16:41

Copilot AI reviewed Nov 12, 2026

View reviewed changes

ColinLeeo added 2 commits November 13, 2026 01:06

Copilot stopped reviewing on behalf of ColinLeeo due to an error November 12, 2026 17:41
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 17:50
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:52
Workflow completed but conclusion is not available. This may indicate rate limiting or API issues.

ColinLeeo merged commit 3639eae into develop Nov 13, 2026
49 checks passed

Conversation

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

Uh oh!

Motivation

Changes

Tests

Benchmark

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!

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!

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!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ColinLeeo commented Nov 12, 2026 •

edited

Loading

codecov-commenter commented Nov 12, 2026 •

edited

Loading