JackieTien97 commented on code in PR #18409:
URL: https://github.com/apache/iotdb/pull/18409#discussion_r3749226841
##########
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/utils/datastructure/AlignedTVListTest.java:
##########
@@ -586,4 +589,166 @@ public void testCalculateChunkSize() {
Assert.assertEquals(tvList.memoryBinaryChunkSize[0], 0);
Assert.assertEquals(tvList.memoryBinaryChunkSize[1], 0);
}
+
+ @Test
+ public void testMovesUnclonedColumns() {
+ List<TSDataType> dataTypes = new ArrayList<>();
+ for (int i = 0; i < 3; i++) {
+ dataTypes.add(TSDataType.INT64);
+ }
+ AlignedTVList tvList = AlignedTVList.newAlignedList(dataTypes);
+ tvList.putAlignedValue(0, new Object[] {1L, 2L, null});
+
+ Set<Integer> columnsToClone = Collections.singleton(1);
+ long retainedRamSize =
tvList.calculateRamSize(columnsToClone).getRamSize();
+ AlignedTVList.PartialClonePlan partialClonePlan =
tvList.preparePartialClone(columnsToClone);
+ AlignedTVList clonedTvList = partialClonePlan.getCloneList();
+
+ Assert.assertNotNull(tvList.getValues().get(0));
+ Assert.assertNotNull(tvList.getValues().get(2));
+ Assert.assertEquals(1L, tvList.getLongByValueIndex(0, 0));
+ Assert.assertTrue(tvList.isNullValue(0, 2));
+ Assert.assertEquals(2L, clonedTvList.getLongByValueIndex(0, 1));
+
+ partialClonePlan.commit();
+
+ Assert.assertNull(tvList.getValues().get(0));
+ Assert.assertNull(tvList.getValues().get(2));
+ Assert.assertTrue(tvList.isNullValue(0, 0));
+ Assert.assertTrue(tvList.isNullValue(0, 2));
+ Assert.assertEquals(1L, clonedTvList.getLongByValueIndex(0, 0));
+ Assert.assertEquals(2L, clonedTvList.getLongByValueIndex(0, 1));
+ Assert.assertTrue(clonedTvList.isNullValue(0, 2));
+ Assert.assertEquals(retainedRamSize,
tvList.calculateRamSize().getRamSize());
+ }
+
+ @Test
+ public void testPartialRamSizeScalesWithRetainedColumns() {
+ int columnCount = 256;
+ List<TSDataType> dataTypes = new ArrayList<>(columnCount);
+ Object[] values = new Object[columnCount];
+ for (int i = 0; i < columnCount; i++) {
+ dataTypes.add(TSDataType.INT64);
+ values[i] = (long) i;
+ }
+
+ AlignedTVList tvList = AlignedTVList.newAlignedList(dataTypes);
+ tvList.putAlignedValue(1, values);
+ Set<Integer> retainedColumns = Collections.singleton(0);
+ long retainedRamSize =
tvList.calculateRamSize(retainedColumns).getRamSize();
+ long fullRamSize = tvList.calculateRamSize().getRamSize();
+
+ // Only the retained column's materialized arrays are charged, so keeping
1 of 256 columns
+ // must cost far less than the full list.
+ Assert.assertTrue(retainedRamSize < fullRamSize / 64);
Review Comment:
**[P1] Restore the focused unit suite after changing the RAM model**
At the latest head, `mvn test -pl iotdb-core/datanode -am
-Dtest=AlignedTVListTest,FragmentInstanceExecutionTest
-Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false` runs the
end-to-end fragment tests successfully but fails 5 of the 23
`AlignedTVListTest` cases. This assertion can no longer hold because the
retained source intentionally keeps an N-wide fixed container baseline; four
existing delta/absolute-size assertions likewise omit the newly added one-time
container costs. Please update the affected invariants and expectations for the
new model (including a fixed N-wide baseline plus the variable retained
payload) and make the focused suite pass rather than weakening this ratio in
isolation.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/utils/datastructure/AlignedTVList.java:
##########
@@ -212,39 +269,190 @@ public synchronized AlignedTVList cloneForFlushSort() {
public synchronized AlignedTVList clone() {
AlignedTVList cloneList = AlignedTVList.newAlignedList(new
ArrayList<>(dataTypes));
cloneAs(cloneList);
- cloneList.timeDeletedCnt = this.timeDeletedCnt;
- System.arraycopy(
- memoryBinaryChunkSize, 0, cloneList.memoryBinaryChunkSize, 0,
dataTypes.size());
+ cloneColumnDataTo(cloneList, null);
+ cloneList.materializedValueArrayCounts =
+ Arrays.copyOf(materializedValueArrayCounts,
materializedValueArrayCounts.length);
+ cloneList.materializedValueArrayMemCost = materializedValueArrayMemCost;
+ return cloneList;
+ }
+
+ /**
+ * Prepare a partial clone without changing this TVList. The returned plan
must be committed only
+ * after the query-memory reservation succeeds.
+ */
+ public synchronized PartialClonePlan preparePartialClone(Set<Integer>
columnsToClone) {
+ Set<Integer> retainedColumns =
+ new HashSet<>(
+ Objects.requireNonNull(
+ columnsToClone,
+
DataNodeMiscMessages.EXCEPTION_COLUMNSTOCLONE_CANNOT_BE_NULL_458FDF37));
+ AlignedTVList cloneList = AlignedTVList.newAlignedList(new
ArrayList<>(dataTypes), false);
+ // Pre-create the inner value lists for the retained columns; the other
slots stay null until
+ // the ownership transfer moves the source columns into place.
for (int i = 0; i < values.size(); i++) {
- // Clone value
+ if (retainedColumns.contains(i)) {
+ cloneList.values.set(i, new ArrayList<>(values.get(i).size()));
+ }
+ }
+ cloneAs(cloneList);
+ cloneColumnDataTo(cloneList, retainedColumns);
+ return prepareMovePlan(cloneList, retainedColumns);
+ }
+
+ @SuppressWarnings("unchecked")
+ private PartialClonePlan prepareMovePlan(AlignedTVList cloneList,
Set<Integer> retainedColumns) {
+ Objects.requireNonNull(
+ cloneList,
DataNodeMiscMessages.EXCEPTION_CLONELIST_CANNOT_BE_NULL_47AEEA8F);
+ int columnCount = values.size();
+ if (cloneList.values.size() != columnCount
+ || cloneList.memoryBinaryChunkSize.length !=
memoryBinaryChunkSize.length) {
+ throw new IllegalStateException(
+ DataNodeMiscMessages
+
.EXCEPTION_TARGET_ALIGNEDTVLIST_HAS_INCOMPATIBLE_COLUMN_CONTAINERS_31FAC613);
+ }
+
+ List<Object>[] valueColumnsToMove = (List<Object>[]) new
List<?>[columnCount];
+ List<BitMap>[] bitmapColumnsToMove = (List<BitMap>[]) new
List<?>[columnCount];
Review Comment:
**[P1] Account for the remaining O(N) move-plan arrays**
The new partial constructor removes the per-column empty inner lists, but
preparation still allocates these two `columnCount` arrays after the caller has
reserved only `listRamInfo`, which represents the final retained source. The
prepared clone already consumes roughly that reservation, while these arrays
coexist with it and are absent from both final lists' accounting. For a wide
aligned device this leaves an unprotected `2 * N * NUM_BYTES_OBJECT_REF` peak,
so the admission-control issue is only partially fixed. Please either eliminate
these arrays (for example, validate first and move by the retained-column set
during an allocation-free commit) or reserve and release their exact temporary
cost, with a wide peak-accounting test.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/utils/datastructure/AlignedTVList.java:
##########
@@ -188,7 +246,6 @@ public TVList getTvListByColumnIndex(
(long) materializedArrayCount *
valueListArrayMemCost(dataTypeList.get(i));
Review Comment:
**[P2] Do not count projected value-slot references twice**
`calculateContainerRamCost` now charges every reference in each retained
`columnValues` backing array. The normal materialization path was accordingly
changed to cache only `primitiveArrayMemCost`, but this projected-list path
still caches `valueListArrayMemCost`, which includes the slot reference.
Consequently every materialized projected block is overcounted by
`NUM_BYTES_OBJECT_REF`, making `calculateRamSize()` depend on how the list was
constructed. Please use `primitiveArrayMemCost` here as well and add a
projected-list RAM assertion under the new container model.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
