fix(cpp): never seal empty chunks and fix dangling ref in parallel tablet write - #909
Open
kkzi wants to merge 4 commits into
Open
fix(cpp): never seal empty chunks and fix dangling ref in parallel tablet write#909kkzi wants to merge 4 commits into
kkzi wants to merge 4 commits into
Conversation
…blet write Two fixes in TsFileWriter: 1. flush_chunk_group / flush_chunk_group_encoded: skip registered-but-empty measurement columns. A measurement that received no data in a window used to be sealed as an EMPTY chunk (count=0, dataSize=0). Java readers (TsFileSequenceReader self-check) treat such a file as crashed and refuse to load it. Mirror the aligned branch's existing hasData() check so empty columns never produce a chunk. 2. write_table (aligned parallel path): the submitted tasks run asynchronously on the thread pool, but the lambdas captured the loop variables (ctx, vt) by reference. Once the loop advances, every queued task reads the same / already-destroyed loop variable. Capture the per-iteration addresses by value instead.
Contributor
There was a problem hiding this comment.
Pull request overview
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.
Fixes two correctness issues in the C++ TsFile writer: avoiding creation of invalid empty chunks in the non-aligned flush path, and preventing dangling reference captures in the aligned parallel tablet write path.
Changes:
- Skip sealing registered-but-empty chunk writers in
flush_chunk_group()andflush_chunk_group_encoded()viaChunkWriter::hasData(). - Capture per-iteration
DeviceWriteCtx/ValueTaskpointers by value when submitting thread-pool tasks inwrite_table().
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Author
|
Pushed CI runs for the new commit are waiting for approval ( |
Contributor
|
Hi @Kzzi, thanks for creating the PR. |
added 2 commits
August 25, 2026 23:01
…let write Covers the two fixes in this PR (issue apache#908): 1. Never seal registered-but-empty measurements as count=0 chunks (non-aligned flush path). Seven tests drive write_tablet, write_record, multi-window flush, empty-window-then-write, sibling devices, and mixed aligned/non-aligned devices, asserting via TsFileReader timeseries metadata that an unwritten measurement is absent from the file while surviving columns carry real statistics. 2. Parallel aligned tablet write capture-by-value. Five tests drive the thread-pool path in write_table() through TsFileTableWriter with multiple devices x columns x rows crossing page boundaries, and verify every cell round-trips (row completeness, per-cell values, tag correctness). Verified: with both fixes temporarily reverted, the empty-chunk tests fail (empty column sealed, meta.size()==3); with the fixes in place all 12 tests pass (10 consecutive runs) and the full TsFile_Test suite (762 tests) passes.
…gressions Fix 1 coverage: - memory-threshold auto-flush path: check_memory_size_and_may_flush_chunks() is a second entry into flush_chunk_group that the explicit-flush tests never drive; drive it via a shrunk chunk_group_size_threshold_. - partially-null column counter-check: the hasData() guard must not skip columns with some nulls (null-bitmap fallback path) — only fully empty ones. - TEXT column variant: empty TEXT takes the string write path, distinct from the fixed-width paths used before. Fix 2 coverage: - tiny page size (8 points) so pool-thread tasks seal pages repeatedly and exercise the initial_page_points continuation across batches. - thread-pool boundary configs: 1-thread pool (serialized workers, worst case for slot aliasing) and 8-thread pool, via set_thread_count(). Verified: auto-flush and TEXT tests fail with the hasData() guard reverted; all 17 tests pass with the fixes (and the full TsFile_Test suite, 767 tests).
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.
Fixes #908
Two fixes in
cpp/src/writer/tsfile_writer.cc1. Never seal empty chunks (non-aligned flush path)
flush_chunk_group()/flush_chunk_group_encoded()now skip registered-but-empty measurement columns via the existingChunkWriter::hasData()check — mirroring what the aligned branch already does.Before: a measurement that received no data in a window was sealed as an EMPTY chunk (
count=0, dataSize=0). Java readers (TsFileSequenceReaderself-check /TsFileSketchTool) treat the whole file as crashed and refuse to load it, even though the other chunks are valid.After: empty columns produce no chunk at all.
2. Fix dangling reference capture in parallel aligned tablet write
In
write_table()'s aligned parallel path, the tasks submitted to the thread pool previously captured the loop variables (ctx,vt) by reference. Since the pool executes tasks asynchronously (after the loop advances/exits), the references dangle and tasks can read wrong or out-of-scope state. The fix captures the per-iteration addresses by value (ctx_ptr/vt_ptr), so each task reads its ownDeviceWriteCtx/ValueTask.Verification
tsfile_writer.ccchanged, no build config touched).count=0chunk), andTsFileSketchTool/print-tsfile.batread it completely (END of TsFile,TsFile Sketch End, no errors).