-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix aligned flush values after time deletion #18634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -722,6 +722,10 @@ private void handleEncodingWithoutDeletedMeasurements( | |
| alignedWorkingListForFlush.getValueIndex(sortedRowIndex))) { | ||
| continue; | ||
| } | ||
| // Keep value pages aligned with the time page when an entire timestamp is deleted. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Skip time-deleted rows while encoding aligned value pages so value positions remain aligned with the time page. This fixes the reproduced flush regression where later values shifted after DELETE plus FLUSH. |
||
| if (alignedWorkingListForFlush.isTimeDeleted(sortedRowIndex)) { | ||
| continue; | ||
| } | ||
| // skip time duplicated rows | ||
| long time = alignedWorkingListForFlush.getTime(sortedRowIndex); | ||
| if (Objects.nonNull(timeDuplicateInfo)) { | ||
|
|
@@ -1116,6 +1120,10 @@ private void handleEncodingWithDeletedMeasurements( | |
| alignedWorkingListForFlush.getValueIndex(sortedRowIndex))) { | ||
| continue; | ||
| } | ||
| // Keep value pages aligned with the time page when an entire timestamp is deleted. | ||
| if (alignedWorkingListForFlush.isTimeDeleted(sortedRowIndex)) { | ||
| continue; | ||
| } | ||
| // skip time duplicated rows | ||
| long time = alignedWorkingListForFlush.getTime(sortedRowIndex); | ||
| if (Objects.nonNull(timeDuplicateInfo)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -272,6 +272,80 @@ public void testAlignedFastPathKeepsPagesAndValuesAlignedAfterPartialSegmentSort | |
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testAlignedFlushKeepsValuesAlignedAfterTimeDeletion() throws IOException { | ||
| // Deleted rows must be omitted from both time and value pages, including after sorting. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests cover both aligned flush encoding paths and page/chunk boundaries, proving deleted timestamps do not shift values and column-remapping remains correct. |
||
| checkAlignedFlushAfterTimeDeletion(false); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAlignedFlushKeepsValuesAlignedAfterTimeAndColumnDeletion() throws IOException { | ||
| // Removing the first measurement also exercises the remapped value-column encoding path. | ||
| checkAlignedFlushAfterTimeDeletion(true); | ||
| } | ||
|
|
||
| private void checkAlignedFlushAfterTimeDeletion(boolean removeColumn) throws IOException { | ||
| for (int pageSize : new int[] {2, 100}) { | ||
| List<IMeasurementSchema> schemas = | ||
| Arrays.asList( | ||
| new MeasurementSchema("s0", TSDataType.INT32, TSEncoding.PLAIN), | ||
| new MeasurementSchema("s1", TSDataType.INT64, TSEncoding.PLAIN)); | ||
| AlignedWritableMemChunk memChunk = new AlignedWritableMemChunk(schemas, false); | ||
| String alignedFilePath = | ||
| TestConstant.OUTPUT_DATA_DIR.concat("testAlignedTimeDeletion" + pageSize + ".tsfile"); | ||
| try { | ||
| for (int time : new int[] {4, 1, 6, 2, 5, 3}) { | ||
| memChunk.putAlignedRow(time, new Object[] {time, time * 10L}); | ||
| } | ||
| memChunk.deleteTime(2, 2); | ||
| memChunk.deleteTime(6, 6); | ||
| if (removeColumn) { | ||
| memChunk.removeColumn("s0"); | ||
| } | ||
| memChunk.sortTvListForFlush(); | ||
|
|
||
| BlockingQueue<Object> ioTaskQueue = new LinkedBlockingQueue<>(); | ||
| // Cover a single page and boundaries between pages and chunks. | ||
| memChunk.encodeWorkingAlignedTVList(ioTaskQueue, pageSize + 1, pageSize); | ||
| try (TsFileIOWriter alignedWriter = new TsFileIOWriter(new File(alignedFilePath))) { | ||
| alignedWriter.startChunkGroup(IDeviceID.Factory.DEFAULT_FACTORY.create("root.d")); | ||
| Object task; | ||
| while ((task = ioTaskQueue.poll()) != null) { | ||
| if (task instanceof IChunkWriter chunkWriter) { | ||
| chunkWriter.writeToFileWriter(alignedWriter); | ||
| } | ||
| } | ||
| alignedWriter.endChunkGroup(); | ||
| alignedWriter.endFile(); | ||
| } | ||
|
|
||
| try (TsFileSequenceReader sequenceReader = new TsFileSequenceReader(alignedFilePath); | ||
| TsFileReader fileReader = new TsFileReader(sequenceReader)) { | ||
| List<Path> paths = new ArrayList<>(); | ||
| paths.add(new Path("root.d", "s1", false)); | ||
| if (!removeColumn) { | ||
| paths.add(new Path("root.d", "s0", false)); | ||
| } | ||
| QueryDataSet dataSet = fileReader.query(QueryExpression.create(paths, null)); | ||
| for (int time : new int[] {1, 3, 4, 5}) { | ||
| assertTrue(dataSet.hasNext()); | ||
| RowRecord row = dataSet.next(); | ||
| assertEquals(time, row.getTimestamp()); | ||
| assertEquals(TSDataType.INT64, row.getFields().get(0).getDataType()); | ||
| assertEquals(time * 10L, row.getFields().get(0).getLongV()); | ||
| if (!removeColumn) { | ||
| assertEquals(TSDataType.INT32, row.getFields().get(1).getDataType()); | ||
| assertEquals(time, row.getFields().get(1).getIntV()); | ||
| } | ||
| } | ||
| assertFalse(dataSet.hasNext()); | ||
| } | ||
| } finally { | ||
| memChunk.release(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testAlignedFastPathEncodesUnmaterializedSegments() throws Exception { | ||
| // Exercise all six value representations with null/dense/null segments, partial nulls, an | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This integration test reproduces the case1 SQL sequence and verifies row counts and exact values before and after FLUSH, guarding the user-visible regression.