JackieTien97 commented on code in PR #18409:
URL: https://github.com/apache/iotdb/pull/18409#discussion_r3747590327
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/memtable/AbstractWritableMemChunk.java:
##########
@@ -109,19 +111,39 @@ protected void maybeReleaseTvList(TVList tvList) {
}
}
+ /**
+ * Try to release the TVList. If there are active queries, transfer memory
ownership to the first
+ * query. For AlignedTVList, this will release non-query columns before
transferring to reduce
+ * memory footprint.
+ */
private void tryReleaseTvList(TVList tvList) {
- long tvListRamSize = tvList.calculateRamSize().getRamSize();
tvList.lockQueryList();
try {
if (tvList.getQueryContextSet().isEmpty()) {
tvList.clear();
} else {
QueryContext firstQuery =
tvList.getQueryContextSet().iterator().next();
+
+ // For AlignedTVList with active queries, release non-query columns
before
+ // transferring memory ownership to reduce memory footprint.
+ if (tvList instanceof AlignedTVList) {
+ AlignedTVList alignedTVList = (AlignedTVList) tvList;
+
+ // Get the union of all columns accessed by queries
+ Set<Integer> accessedColumns =
alignedTVList.getAccessedColumnsForQuery();
+
+ if (accessedColumns != null && !accessedColumns.isEmpty()) {
Review Comment:
**[P2] Preserve the meaning of a tracked empty column set**
A table-model time-only scan legitimately records an empty
`columnIndexList`. This guard, together with
`AlignedTVList.releaseNonQueryColumns` returning early for an empty set, keeps
every value array when the flushed TVList is transferred to that query. The
most selective case therefore retains the full wide aligned list and defeats
this PR's memory-reduction goal.
Please distinguish "tracked and empty" (time-only: release all value
columns) from "not tracked/unsupported context" (unknown: conservatively retain
all columns), and add a focused table-model time-only ownership-transfer test.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/utils/datastructure/AlignedTVList.java:
##########
@@ -1137,22 +1410,51 @@ public synchronized RamInfo calculateRamSize() {
new ArrayList<>(dataTypes));
}
+ public synchronized RamInfo calculateRamSize(Set<Integer> columnsToClone) {
+ return new RamInfo(
+ timestamps.size(),
+ alignedTvListArrayMemCost(columnsToClone),
+ getRamSize(columnsToClone),
+ rowCount,
+ new ArrayList<>(dataTypes));
+ }
+
public synchronized long getRamSize() {
return (long) timestamps.size() *
alignedTvListArrayMemCostWithoutPrimitiveArrays()
+ materializedValueArrayMemCost
+ materializedBitmapMemoryCost;
}
- private static long calculateBitmapRamCost(List<List<BitMap>> bitMaps) {
+ public synchronized long getRamSize(Set<Integer> columnsToClone) {
+ long size =
+ (long) timestamps.size() *
alignedTvListArrayMemCostWithoutPrimitiveArrays(columnsToClone);
+ for (int i = 0; i < dataTypes.size(); i++) {
+ if (columnsToClone != null && !columnsToClone.contains(i)) {
+ continue;
+ }
+ TSDataType dataType = dataTypes.get(i);
+ if (dataType != null) {
+ size += (long) materializedValueArrayCounts[i] *
valueListArrayMemCost(dataType);
+ }
+ }
+ return size + calculateBitmapRamCost(bitMaps, columnsToClone);
Review Comment:
**[P1] Include the retained wide-column containers in query memory
accounting**
The final follow-up in #18394 (`c4f31171`) added `calculateContainerRamCost`
because, after a partial move, the query-owned source TVList still retains
N-wide structures such as `dataTypes`, `memoryBinaryChunkSize`, and the outer
`values`/`bitMaps` containers. On master it also retains the N-wide
`materializedValueArrayCounts` array. This implementation charges only
timestamps, materialized value arrays, and bitmaps, so a query touching one
column of a very wide aligned device can under-reserve O(N) memory even though
the clone path claims its transient allocation is protected by admission
control.
Please port that final accounting fix, adapted to master's lazy-allocation
fields, and restore an equivalent wide-container assertion (plus any affected
RAM expectations). Otherwise this is not semantically equivalent to the final
#18394 implementation. Reference:
https://github.com/apache/iotdb/commit/c4f31171b69d38f42ae5c39c29226438dccec03f
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/utils/datastructure/AlignedTVList.java:
##########
@@ -212,39 +260,165 @@ 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());
- for (int i = 0; i < values.size(); i++) {
- // Clone value
+ 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, "columnsToClone
cannot be null"));
Review Comment:
**[P1] Move all new exception text into the compile-time i18n message
classes**
This raw message remains English under `-P with-zh-locale` and violates the
repository rule for `requireNonNull`, thrown exceptions, and `String.format`
templates. The same issue appears in the new messages around lines 285, 289,
302, 306, 315, 1002, 1081, and 1349-1352.
Please add one full-template constant per message to the appropriate
`*Messages` class in both `src/main/i18n/en` and `src/main/i18n/zh`, preserving
each `%d`, and reference those constants from `AlignedTVList`.
##########
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/queryengine/execution/fragment/FragmentInstanceExecutionTest.java:
##########
@@ -308,12 +307,29 @@ private IMemTable createMemTable(String deviceId, String
measurementId)
int rows = 100;
for (int i = 0; i < 100; i++) {
memTable.write(
- DeviceIDFactory.getInstance().getDeviceID(new PartialPath(deviceId)),
+ IDeviceID.Factory.DEFAULT_FACTORY.create(deviceId),
Collections.singletonList(
new MeasurementSchema(measurementId, TSDataType.INT32,
TSEncoding.PLAIN)),
rows - i - 1,
new Object[] {i + 10});
}
return memTable;
}
+
+ private IMemTable createMemTable(String deviceId, List<IMeasurementSchema>
schemaList)
Review Comment:
**[P2] Restore the end-to-end aligned partial-query test**
This helper is unused, so the PR description's claim that
`FragmentInstanceExecutionTest` verifies partial-column aligned queries is not
true in the final diff. The source PR removed that test "temporarily" in
`ec4ae27a`, but it was never restored. The remaining tests cover
`AlignedTVList` mechanics, not the full `prepareTvListMapForQuery`
clone-and-swap path with real query contexts.
Please add the intended end-to-end test: keep one query active on an
unsorted aligned working TVList, trigger a second partial-column query, and
verify both query results plus the ownership/memory transition. Reference:
https://github.com/apache/iotdb/commit/ec4ae27a5caa4bea9ec68223eb628d12afb93948
--
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]
