clone partial columns of aligned tvlist for query - #18409
Conversation
shuwenwei
changed the title
…ager test fakes The cherry-pick added the releaseMemoryImmediately(long) method to the calc-commons MemoryReservationManager interface, but four test classes that implement the interface directly were not updated, breaking test compilation. Add the missing method to each fake, mirroring the semantics of the existing releaseMemoryCumulatively implementation in each class.
- ResourceByPathUtilsTest: getResourceInstance(IFullPath) requires a NonAlignedFullPath; MeasurementPath does not implement IFullPath, so the test no longer compiled. Build the NonAlignedFullPath from IDeviceID.Factory.DEFAULT_FACTORY and a MeasurementSchema. - FragmentInstanceExecutionTest: use IDeviceID.Factory.DEFAULT_FACTORY.create directly instead of DeviceIDFactory.getInstance().getDeviceID(new PartialPath(...)), matching the query-side NonAlignedFullPath construction in the same file, and drop the now-unused imports.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18409 +/- ##
============================================
+ Coverage 43.61% 43.66% +0.04%
Complexity 374 374
============================================
Files 5393 5399 +6
Lines 385958 386622 +664
Branches 50267 50406 +139
============================================
+ Hits 168337 168803 +466
- Misses 217621 217819 +198 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| // release TVList/AlignedTVList owned by current query | ||
| releaseTVListOwnedByQuery(); | ||
| alignedTVListColumnAccessMap.clear(); |
There was a problem hiding this comment.
| alignedTVListColumnAccessMap.clear(); | |
| alignedTVListColumnAccessMap = null; |
set to null may be better and more effecient
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
Requesting changes because this cherry-pick is not yet semantically equivalent to the final dev/1.3 implementation:
- The final wide-column container memory-accounting fix from #18394 is missing.
- The master table-model time-only case retains every value column during flush ownership transfer.
- Several newly added invariant/error messages bypass the project's compile-time i18n mechanism.
- The end-to-end aligned partial-query test claimed in the PR description is absent.
The core candidate recheck, atomic clone/move/swap, reservation rollback, and temporary flush-list registration fixes appear to have been carried over correctly. I also ran the focused test set locally (43 DataNode tests and 6 calc-commons tests); all passed. Please address the inline findings before merging.
| size += (long) materializedValueArrayCounts[i] * valueListArrayMemCost(dataType); | ||
| } | ||
| } | ||
| return size + calculateBitmapRamCost(bitMaps, columnsToClone); |
There was a problem hiding this 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: c4f3117
| // Get the union of all columns accessed by queries | ||
| Set<Integer> accessedColumns = alignedTVList.getAccessedColumnsForQuery(); | ||
|
|
||
| if (accessedColumns != null && !accessedColumns.isEmpty()) { |
There was a problem hiding this 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.
| */ | ||
| public synchronized PartialClonePlan preparePartialClone(Set<Integer> columnsToClone) { | ||
| Set<Integer> retainedColumns = | ||
| new HashSet<>(Objects.requireNonNull(columnsToClone, "columnsToClone cannot be null")); |
There was a problem hiding this 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.
| return memTable; | ||
| } | ||
|
|
||
| private IMemTable createMemTable(String deviceId, List<IMeasurementSchema> schemaList) |
There was a problem hiding this 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: ec4ae27
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
Requesting changes for two additional master-specific memory-accounting gaps found after rechecking the partial-clone lifecycle. The current reservation and cleanup calculations reconcile numerically, but they reconcile against an incomplete model: lazy null backing slots are still omitted, and the preparation phase creates an unreserved O(N) transient peak. Please address both inline findings in addition to the previously reported retained-container accounting issue.
| } | ||
| TSDataType dataType = dataTypes.get(i); | ||
| if (dataType != null) { | ||
| size += (long) materializedValueArrayCounts[i] * valueListArrayMemCost(dataType); |
There was a problem hiding this comment.
[P1] Count lazy null backing slots independently of materialized arrays
On master, a retained value column can have B block entries in its inner ArrayList but only M materialized primitive arrays. This line charges NUM_BYTES_OBJECT_REF only M times via valueListArrayMemCost; meanwhile alignedTvListArrayMemCostWithoutPrimitiveArrays subtracts that per-column reference for every block. The B-M null slots in the inner backing Object[] are therefore uncharged. Porting dev/1.3's calculateContainerRamCost verbatim would not fix this, because its listRamCostWithoutReferences intentionally omits the backing-array references.
Please adapt the container calculation to master's lazy layout so every inner-list slot is counted exactly once while primitive payload/header cost remains based on the materialized count. Add a sparse multi-block case where a retained column has both materialized and all-null blocks; it should verify the calculated retained size as well as reservation/cleanup equality.
| throw new IllegalStateException("Target AlignedTVList has incompatible column containers"); | ||
| } | ||
|
|
||
| List<Object>[] valueColumnsToMove = (List<Object>[]) new List<?>[columnCount]; |
There was a problem hiding this comment.
[P1] Cover the O(N) partial-clone preparation peak before allocating it
The caller reserves only the final retained-source size before entering this method. However, newAlignedList has already created an inner ArrayList for every column (potentially preallocating getDefaultArrayNum() slots), and these two N-entry move arrays are allocated as additional preparation state. For every non-retained column, the clone's empty inner list is replaced during commit and becomes garbage, so this transient O(N) allocation is absent from both the final source size and the existing reservation. This remains true even after adding the retained-container cost requested in the earlier thread, so the claim that the transient clone increase is protected by admission control is not currently valid for wide aligned devices.
Please either construct a partial clone with null outer slots and allocate inner lists only for retained columns, or estimate and reserve the preparation overhead separately and release that temporary reservation after commit/rollback. Add a wide sparse-column test that exercises this peak-accounting path.
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
Requesting changes after rechecking the latest head (f7df7b6). The recent updates address the tracked-empty/time-only case, i18n, lazy null-slot accounting, retained N-wide containers, and the missing end-to-end clone path. However, the preparation phase still has an unreserved O(N) peak, projected AlignedTVList instances now double-count materialized slot references, and the focused AlignedTVListTest suite fails in five places because the new container-memory model has not been reconciled with its invariants. Please address the inline findings before merging.
| List<Object>[] valueColumnsToMove = (List<Object>[]) new List<?>[columnCount]; | ||
| List<BitMap>[] bitmapColumnsToMove = (List<BitMap>[]) new List<?>[columnCount]; |
There was a problem hiding this 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.
There was a problem hiding this comment.
Fixed in a35f076 by taking the "eliminate these arrays" option: prepareMovePlan now validates the move without allocating any O(N) structure (all existing checks are kept), and commitPartialClone derives the moved columns from the retained set (retainedColumns.contains(i) == false), so the commit is fully allocation-free.
On the question of whether the two arrays deserve a separate reservation: after the earlier change (f7df7b6) removed the N per-column inner ArrayLists by creating the clone with null outer slots and pre-creating inner lists only for retained columns, these two columnCount reference arrays (2 x N x NUM_BYTES_OBJECT_REF) were the only remaining transient allocation between the reservation and the commit. They are now gone entirely, so the preparation phase between reserveMemoryCumulatively(listRamInfo) and commit() is allocation-free and the admission-control claim holds by construction — there is no temporary O(N) state left to reserve or release. The existing partial-clone and end-to-end FragmentInstanceExecution tests exercise preparePartialClone + commit on sparse/wide layouts and pass.
| @@ -188,7 +246,6 @@ public TVList getTvListByColumnIndex( | |||
| (long) materializedArrayCount * valueListArrayMemCost(dataTypeList.get(i)); | |||
There was a problem hiding this 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.
|
|
||
| // 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); |
There was a problem hiding this 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.
|
2 participants
Cherry-picked from #18394 ([To dev/1.3] clone partial columns of aligned tvlist for query).
Commit:
edeb7c5276For queries that touch only a few columns of a wide aligned device, clone only the accessed columns of the working
AlignedTVListinstead of the whole list, then swap the working list so the unaccessed columns are moved out. This dramatically reduces the memory retained by such queries on wide aligned devices.This cherry-pick also carries over the follow-up fixes from the dev/1.3 PR:
prepareTvListMapForQueryretries under thememChunklock after acquiring the candidate'squeryListLock, so a swapped working list can never be operated on while holding a stale lock.memChunkcritical section.RamInfoaccounting keeps reservation and cleanup sizes consistent.Adapted to the current master APIs (IFullPath/AlignedFullPath, ModEntry, table-model chunk metadata, i18n messages) during conflict resolution.