Conversation
…surements Every value column of an aligned chunk group has to consume exactly one row per time column row. write_record_aligned() / write_tablet_aligned() only advanced the columns that the incoming record/tablet carried, so a measurement missing from a row left its column one row short: its not-null bitmap started at the wrong row index and the values were paired with the earliest timestamps of the page on read, and the statistics recomputed after recovery described the shifted rows. - create the value chunk writer when the measurement is registered on an aligned device, so every registered measurement takes part from row 0 - write NULL rows for the measurements a record/tablet does not carry (Java: AlignedChunkGroupWriterImpl#write -> writeEmptyDataInOneRow) - reject registering a new measurement once rows have been written, which would need backfilled rows/pages; Java does not allow expanding an aligned device either - ValuePageWriter::write_null_rows() / ValueChunkWriter::write_null_batch() advance the column by NULL rows and keep page boundaries in step with the time column - a record repeating a measurement now advances that column once (last point wins) instead of running ahead of the time column Tests: TsFileWriterTest.AlignedRecordMissingMeasurementsStayRowAligned, AlignedRecordMissingMeasurementsAcrossPages, AlignedTabletMissingColumnStaysRowAligned, AlignedRecordDuplicateMeasurementWritesOneRow, AlignedRegisterAfterWriteIsRejected and RestorableTsFileIOWriterTest.AlignedTimeseriesRecoverAndWriteNullValue.
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.
Bug
For an aligned (tree-model) device, every value column has to consume exactly one row per time-column row. The C++ writer only advanced the columns that the incoming row actually carried:
TsFileWriter::write_record_aligned()built its writer list fromrecord.points_, while the time chunk was written for every record;TsFileWriter::write_tablet_aligned()only advanced the tablet's own columns.So a measurement that was missing from a row left its column one row short. Its not-null bitmap then started at the wrong row index, and on read the values were paired with the earliest timestamps of the page while the tail rows came back as
NULL; the statistics thatRestorableTsFileIOWriterrecomputes after a crash described those shifted rows too (so time-filter pruning could drop or return the wrong range).Java does not have this problem —
AlignedChunkGroupWriterImpl#writefills the measurements a row does not carry:Reproducer (same data, same C++ reader; Java-written file on the left, C++-written file on the right) — a device with
s1/s2where even rows only writes1and odd rows only writes2:The tablet path was broken the same way: a column that first showed up in the second tablet got its values shifted to the beginning of the page.
Fix
Make the aligned invariant hold: every registered measurement of an aligned device advances exactly one row per row written, with a
NULLbit when the row carries no value for it.TsFileWriter::register_timeseries()now creates theValueChunkWriterof an aligned measurement at registration time (ensure_aligned_value_chunk_writer()), so the column takes part from row 0.write_record_aligned()iterates the device schema instead of only the record's points and writesNULLrows for the measurements the record does not carry. Values are still written in record order; the mapping is by measurement name, so the order of the points inside a record (and the order of the columns) does not matter.write_tablet_aligned()writesNULLrows for the registered measurements a tablet does not carry, and both paths now include those columns in the page-seal lockstep.ValuePageWriter::write_null_rows()/ValueChunkWriter::write_null_batch()implement theNULLwalk: one zero bit per row, no value bytes, no statistic update, page boundaries split onpage_writer_max_point_num_likewrite_batch()so the page lists of the time column and of every value column stay in step.E_INVALID_ARGinstead of silently producing a shifted chunk group — it would need backfilled rows/pages, which the current page writer cannot express (Java does not allow expanding an aligned device either).Behaviour notes / limits
count == 0) instead of being absent from the metadata. This matches what the aligned tablet path already did for a column whose values are allNULL.Nvalues insidestart_time..end_timethere is generally more than one candidate). Those files should be regenerated; the stored chunk statistic contradicting the bitmap-derived range can at least be used to detect them.Tests
cpp/test/writer/tsfile_writer_test.ccAlignedRecordMissingMeasurementsStayRowAligned— sparse records, per-row values/timestamps and per-measurement statistics.AlignedRecordMissingMeasurementsAcrossPages— same withpage_writer_max_point_num_ = 7so the NULL padding has to seal pages in lockstep.AlignedTabletMissingColumnStaysRowAligned— tablet path, column introduced by the second tablet.AlignedRecordDuplicateMeasurementWritesOneRowAlignedRecordPointOrderDoesNotMatter— rotatedadd_pointorder per row.AlignedRegisterAfterWriteIsRejectedcpp/test/file/restorable_tsfile_io_writer_test.ccAlignedTimeseriesRecoverAndWriteNullValue— 10 sparse rows, corrupt tail,RestorableTsFileIOWriterrecovery, 10 more sparse rows; assertscount == 10,start/endper measurement and the row positions after recovery.All six new writer tests fail on the unpatched code (verified by rebuilding the same test binary against
develop) and pass with the fix. Full C++ suite: 763 tests, 760 passed, 3 skipped, 0 failed.