Skip to content

fix(cpp): keep aligned value columns row-aligned when a record omits measurements - #968

Open
ColinLeeo wants to merge 1 commit into
apache:developfrom
ColinLeeo:colin/fix-cpp-aligned-record-nulls
Open

ColinLeeo wants to merge 1 commit into
apache:developfrom
ColinLeeo:colin/fix-cpp-aligned-record-nulls

Conversation

@ColinLeeo

Copy link
Copy Markdown
Contributor

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 from record.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 that RestorableTsFileIOWriter recomputes 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#write fills the measurements a row does not carry:

for (Map.Entry<String, ValueChunkWriter> entry : valueChunkWriterMap.entrySet())
  if (!existingMeasurements.contains(entry.getKey())) emptyValueChunkWriters.add(entry.getValue());
...
if (!emptyValueChunkWriters.isEmpty()) writeEmptyDataInOneRow(emptyValueChunkWriters);

Reproducer (same data, same C++ reader; Java-written file on the left, C++-written file on the right) — a device with s1/s2 where even rows only write s1 and odd rows only write s2:

Java                                   C++
time  d1.s1  d1.s2                     time  d1.s1  d1.s2
0     100                              0     100    201
1            201                       1     102    203
2     102                              2     104    205
3            203                       3     106    207
4     104                              4     108    209
5            205                       5
...                                    ...
9            209                       9

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 NULL bit when the row carries no value for it.

  • TsFileWriter::register_timeseries() now creates the ValueChunkWriter of 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 writes NULL rows 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() writes NULL rows 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 the NULL walk: one zero bit per row, no value bytes, no statistic update, page boundaries split on page_writer_max_point_num_ like write_batch() so the page lists of the time column and of every value column stay in step.
  • Registering a new measurement on an aligned device that already has data is now rejected with E_INVALID_ARG instead 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).
  • A record that repeats the same measurement now advances that column once (the last point wins). Previously both points were written, which ran the column ahead of the time column.

Behaviour notes / limits

  • A measurement that is registered but never carries a value now shows up as an all-NULL column (count == 0) instead of being absent from the metadata. This matches what the aligned tablet path already did for a column whose values are all NULL.
  • Files already written by the buggy writer cannot be repaired: the page only stores "the first N rows are non-null", so the original row positions of those N values are not in the file (for N values inside start_time..end_time there 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.cc

  • AlignedRecordMissingMeasurementsStayRowAligned — sparse records, per-row values/timestamps and per-measurement statistics.
  • AlignedRecordMissingMeasurementsAcrossPages — same with page_writer_max_point_num_ = 7 so the NULL padding has to seal pages in lockstep.
  • AlignedTabletMissingColumnStaysRowAligned — tablet path, column introduced by the second tablet.
  • AlignedRecordDuplicateMeasurementWritesOneRow
  • AlignedRecordPointOrderDoesNotMatter — rotated add_point order per row.
  • AlignedRegisterAfterWriteIsRejected

cpp/test/file/restorable_tsfile_io_writer_test.cc

  • AlignedTimeseriesRecoverAndWriteNullValue — 10 sparse rows, corrupt tail, RestorableTsFileIOWriter recovery, 10 more sparse rows; asserts count == 10, start/end per 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.

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant