fix(cpp): never seal empty chunks and fix dangling ref in parallel tablet write - #909
fix(cpp): never seal empty chunks and fix dangling ref in parallel tablet write#909kkzi 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.
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.
|
Pushed CI runs for the new commit are waiting for approval ( |
|
Hi @Kzzi, thanks for creating the PR. |
…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).
|
Hi @ColinLeeo, thanks for the review! Regression tests have been added in 3a4c59f and cf742de (17 tests total, all passing along with the full TsFile_Test suite — 767 tests): Empty-chunk sealing (fix 1) —
Parallel aligned tablet write (fix 2) —
Verified both directions: with the One note for completeness: the parallel-write tests are end-to-end coverage of the thread-pool path rather than a deterministic reproducer of the dangling capture — |
|
CI runs for the new test commits ( |
| // Capture pointers by value: the submitted tasks run on pool | ||
| // threads asynchronously (after this loop returns), so | ||
| // capturing the loop variables by reference would dangle | ||
| // (all tasks would read the same/out-of-scope ctx). |
| row_index * 100 + c) | ||
| << "field s" << c << " corrupted at ts " << ts; | ||
| } | ||
| } |
|
Thanks for the fix, and for the thorough test coverage — the empty-chunk fix (fix #1) looks solid to me. One thing I'd like to flag on fix #2 (the "dangling reference" in the parallel aligned tablet write path): I don't think this bug actually existed in the original code. As Copilot's review also notes, Also, echoing Copilot's other comment: Happy to help with either of these if useful — otherwise this looks good to merge once addressed. |
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).