Repository navigation
Conversation
- take the data model from the data region instead of the "root." device prefix - look up the table TTL with the database name of the data region, because the table cache is keyed by database and table - spotless formatting for the files touched by the previous commit
- CachedSchemaPatternMatcher matches the sources by the model declared in the event - PipeHistoricalDataRegionTsFileAndDeletionSource decides the model by the tsfile database - PipeRawTabletInsertionEvent picks the event parser by isTableModelEvent() - TreeViewTabletProjector drops the device name prefix check
- annotate the tree model only device -> database lookups with @TreeModel - annotate the tree model only database creation with @TreeModel and the table model one with @TableModel - document why the isTableModel flag of the device -> database lookup is false for both callers
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
Two functional regressions reproduced against df3eb05cee0ed12997ba881e4ce70781f62b9227; details, exact JUnit reproductions, and run commands are included inline. Both reproduction tests pass at merge base d20082b1615e6957e77593c9d2623d6fdc843340 and fail on this head. Validation used JDK 17 and clean Maven reactor builds. The latest head's 16 selected Pipe tests passed; the baseline comparison, including the reproductions and related existing tests, passed all 38 tests.
|
|
||
| protected boolean shouldSkipModification(ModEntry modification) { | ||
| if (tables != null && modification instanceof TableDeletionEntry) { | ||
| if (isTableModel() && modification instanceof TableDeletionEntry) { |
There was a problem hiding this comment.
[P2] Preserve the nullable table-filter guard for compaction contexts
A table-model context does not imply that tables has been populated. ReadPointCompactionPerformer.perform() creates its context with createFragmentInstanceContextForCompaction() and sets ignoreAllNullRows to false for table data, but never calls collectTable(). If an input file contains a TableDeletionEntry, this new condition is true and tables.contains(tableName) throws a NullPointerException. Consequently, table-model compaction with the read_point performer fails after deletions. Please retain the tables != null guard: an absent table-name filter must allow the modifications through.
Reproduction: real modification file and the production compaction context factory
Add this test under iotdb-core/datanode/src/test/java/org/apache/iotdb/db/queryengine/execution/fragment/Review18814CompactionModsTest.java:
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.iotdb.db.queryengine.execution.fragment;
import org.apache.iotdb.db.storageengine.dataregion.modification.DeletionPredicate;
import org.apache.iotdb.db.storageengine.dataregion.modification.ModEntry;
import org.apache.iotdb.db.storageengine.dataregion.modification.ModificationFile;
import org.apache.iotdb.db.storageengine.dataregion.modification.TableDeletionEntry;
import org.apache.iotdb.db.storageengine.dataregion.modification.TagPredicate;
import org.apache.iotdb.db.storageengine.dataregion.tsfile.TsFileResource;
import org.apache.tsfile.file.metadata.StringArrayDeviceID;
import org.apache.tsfile.read.common.TimeRange;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import java.io.File;
import java.util.List;
import static org.junit.Assert.assertEquals;
public class Review18814CompactionModsTest {
@Rule public TemporaryFolder temporaryFolder = new TemporaryFolder();
@Test
public void testCompactionContextLoadsTableDeletionsWithoutCollectedTables() throws Exception {
File partition = temporaryFolder.newFolder("db", "0", "0");
TsFileResource resource = new TsFileResource(new File(partition, "1-1-0-0.tsfile"));
StringArrayDeviceID device = new StringArrayDeviceID("table1", "tag1");
TableDeletionEntry deletion =
new TableDeletionEntry(
new DeletionPredicate("table1", new TagPredicate.FullExactMatch(device)),
new TimeRange(1, 10));
try (ModificationFile mods = resource.getModFileForWrite()) {
mods.write(deletion);
}
FragmentInstanceContext context =
FragmentInstanceContext.createFragmentInstanceContextForCompaction(-1);
context.setIgnoreAllNullRows(false);
List<ModEntry> loaded = context.getPathModifications(resource, device, "s1");
assertEquals(1, loaded.size());
assertEquals(deletion, loaded.get(0));
}
}Run with JDK 17:
mvn clean test -pl iotdb-core/datanode -am \
-Dtest=Review18814CompactionModsTest \
-Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=falseActual comparison: the test passes at merge base d20082b1615e6957e77593c9d2623d6fdc843340 and fails at PR head df3eb05cee0ed12997ba881e4ce70781f62b9227 with:
java.lang.NullPointerException: Cannot invoke "java.util.Set.contains(Object)" because "this.tables" is null
at org.apache.iotdb.db.queryengine.execution.fragment.QueryContext.shouldSkipModification(QueryContext.java:150)
at org.apache.iotdb.db.queryengine.execution.fragment.QueryContext.loadAllModificationsFromDisk(QueryContext.java:138)
There was a problem hiding this comment.
Fixed in 36683ba: shouldSkipModification only skips a TableDeletionEntry when a table filter was collected (tables != null), so a table-model context that never collected one (compaction, and also the table last_by aggregation scan) keeps every modification instead of throwing. That NPE is exactly what the CI failure in IoTDBDeletionTableIT.testLastByOverDeletedDeviceStillReturnsSingleNullRow turned out to be: the scan's catch-all wrapped it into a misleading "TsFile may be corrupted" error, and the guard is the fix for it.
| this.isTableModel = | ||
| tableDatabaseName != null && PathUtils.isTableModelDatabase(tableDatabaseName); |
There was a problem hiding this comment.
[P2] Resolve the source model per snapshot instead of per target database
A WAL directory is not necessarily confined to one data model: with SimpleConsensus, WALManager uses ElasticStrategy or RoundRobinStrategy, which can share a WAL node across tree and table DataRegions. For a shared WAL containing snapshots from root.sg and db, specifying -db db is needed for the table data, but this fixed flag now treats the tree snapshot as table data too. It reaches createTableTabletSchema(), issues DESCRIBE for the tree device through the table Session, and fails replay. Choosing a tree database instead cannot correctly replay the table snapshot. The snapshot's source model needs to be resolved per memtable/entry (for example, from source/checkpoint metadata), independently of the target table database.
Reproduction: serialize tree and table snapshots into one WAL file, then replay it
Add the following method to the existing ImportWALTest; it reuses that class's createWALFile, writeWAL, and mockTableSchema helpers and existing imports:
@Test
public void testReview18814MixedModelSnapshotsInSharedWAL() throws Exception {
final PrimitiveMemTable treeMemTable = new PrimitiveMemTable("root.sg", "0");
final StringArrayDeviceID treeDevice = new StringArrayDeviceID("root.sg.d1");
treeMemTable.writeAlignedRow(
treeDevice,
Collections.singletonList(new MeasurementSchema("s1", TSDataType.INT32)),
1,
new Object[] {10});
final PrimitiveMemTable tableMemTable = new PrimitiveMemTable("db", "1");
tableMemTable.writeAlignedRow(
new StringArrayDeviceID("table1", "tag1"),
Collections.singletonList(new MeasurementSchema("s1", TSDataType.INT32)),
2,
new Object[] {20});
final File walFile = createWALFile(0);
writeWAL(walFile, new WALInfoEntry(1, treeMemTable), new WALInfoEntry(2, tableMemTable));
final Session treeSession = mock(Session.class);
final Session tableSession = mock(Session.class);
mockTableSchema(tableSession, "table1", "time", "tag1");
when(tableSession.executeQueryStatement(
"DESCRIBE " + ImportWAL.WALReplayer.quoteIdentifier(treeDevice.getTableName())))
.thenThrow(new StatementExecutionException("Tree device is not a table in db"));
final ImportWAL.ReplayStatistics statistics =
ImportWAL.replayWALFiles(
Collections.singletonList(walFile.toPath()),
new ImportWAL.WALReplayer(treeSession, tableSession, "db"));
assertEquals(2, statistics.getReplayedOperationCount());
verify(treeSession).insertAlignedTablet(any(Tablet.class));
verify(tableSession).insertRelationalTablet(any(Tablet.class));
}The WAL bytes are written and read through the real WAL implementation. Session RPC is mocked: the target database has table1, and the mock rejects a table-schema lookup for the tree device. The expected routing is one insertAlignedTablet on the tree Session and one insertRelationalTablet on the table Session.
Run with JDK 17:
mvn clean test -pl iotdb-core/datanode -am \
'-Dtest=ImportWALTest#testReview18814MixedModelSnapshotsInSharedWAL' \
-Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=falseActual comparison: the test passes at merge base d20082b1615e6957e77593c9d2623d6fdc843340 and fails at PR head df3eb05cee0ed12997ba881e4ce70781f62b9227. On the PR head the unexpected DESCRIBE reaches the mock, throwing StatementExecutionException: Tree device is not a table in db, which is wrapped in WALReplayException before both snapshots can be replayed.
There was a problem hiding this comment.
As you say, this is a pre-existing limitation of the tool rather than something this PR introduces. A WAL entry does not carry a database (for table data the database comes from the client session), so when several table-model databases share one WAL node a single -db/--database is inherently insufficient - and the same is true for a WAL node shared by tree and table regions, where one model per directory cannot describe the content. Solving it needs per-region resolution (WAL checkpoint memTableId -> regionId plus a region -> database mapping) or a database/model marker in the WAL entry, which is beyond this PR's scope, so we are not attempting it here. What the PR does is state the limitation where the operator sees it: the tool now declares in its help header - and repeats whenever the target database of a WAL directory cannot be determined - that WALs written with the ElasticStrategy/RoundRobinStrategy WAL node allocation strategies cannot be imported.
A compaction context is a table model context but it never collects a table filter, so reading a table deletion from the mods file threw a NullPointerException. Skip a modification only when a table filter exists, i.e. restore the null guard.
PipeRealtimeExtractTest passed the data region id as the database name of the inserted events. The data model of such an event is derived from that string, so "1" resolved to the table model and the tree model sources never matched the tree devices, which made the extract listeners time out. Pass the database the test actually writes to.
A compaction input takes its data model from the database directory of the source files, so the table model fixtures, which write table model devices, must not place their files in the tree model database root.testsg. Point them at the table model database testsg, and drop the cases that mix tree and table files in one task, which cannot occur because a database holds a single data model.
ImportWAL resolves the data model of a WAL directory from its target database. Without one the replayer fell back to the tree model, so a table model snapshot was replayed into the tree session instead of failing. Reject a snapshot whose data model is unknown until -db/--database is declared.
…odel This change no longer calls IDeviceID.isTableModel, so it needs the tsfile snapshot that removes it: bump tsfile.version to 2.4.1-261009-SNAPSHOT.
An empty memtable snapshot has nothing to replay, so it must stay ignorable without a declared target database; otherwise a WAL that only holds signals, separators and an empty snapshot stops being deletable after replay.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18814 +/- ##
============================================
+ Coverage 45.56% 45.86% +0.30%
Complexity 712 712
============================================
Files 5483 5498 +15
Lines 396291 397489 +1198
Branches 51567 51744 +177
============================================
+ Hits 180575 182317 +1742
+ Misses 215716 215172 -544 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…orted The ElasticStrategy and RoundRobinStrategy share WAL nodes between regions, so a WAL directory has neither a single database nor a single data model. State that such WALs cannot be imported when the help is printed, and repeat the declaration whenever the target database of a WAL directory cannot be determined.
3 participants
Motivation
IDeviceID.isTableModel()is being removed in apache/tsfile#998. This PR removes its usages in the DataNode and takes the data model from the surrounding context instead.Changes
QueryContext/FragmentInstanceContext).ignoreAllNullRowsis no longer stored inAlignedWritableMemChunk/AbstractMemTable. It is passed explicitly through the chunk, group, memtable and flush APIs, and resolved once where the context is known.UnsealedTsFileRecoverPerformer,TsFilePlanRedoer), andMemTableFlushTasktakes it from its caller.TsFileProcessor#getQueryTimeLowerBoundno longer infers the model from theroot.device prefix, and looks up the table TTL with the database name of the data region.PartitionCache: the device -> database lookups and the database creation that only exist for one data model are marked and documented with@TreeModel/@TableModel.ImportWAL:-db/--databaseis required when it cannot be inferred;ElasticStrategy/RoundRobinStrategyWAL node allocation strategies are not supported, because those strategies share a WAL node between regions and such a directory has neither a single database nor a single data model.QueryContext.shouldSkipModificationkeeps the nullable table-filter guard (tables != null). A table-model context - compaction, and the tablelast_byaggregation scan - does not necessarily collect a table filter, so reading aTableDeletionEntrywithout the guard throws aNullPointerException; that is whatIoTDBDeletionTableIT#testLastByOverDeletedDeviceStillReturnsSingleNullRowhit in CI.testsg), the cases that fed tree-model and table-model files into one compaction task are removed (a database only ever holds one data model), and the realtime extract test passes the database it actually writes to.tsfile.versionis bumped to the snapshot that removesIDeviceID.isTableModel.Notes
IDeviceID.isTableModel(refactor: remove IDeviceID.isTableModel tsfile#998); this branch already uses the matching tsfile snapshot.ImportWALresolves the database and the data model per WAL directory, so a WAL node shared by several regions (tree and table, or several table databases) cannot be imported correctly - a WAL entry carries no database, and for table data the database comes from the client session. This predates the PR and is now declared by the tool itself (see above); resolving it needs per-region resolution through the WAL checkpoint plus a region -> database mapping, or a database/model marker in the WAL entry.