fix: reserve memory for sorting indices during query execution #16959
Conversation
shizy818
force-pushed
the
sort_mem_1.3
branch
from
c0b92d8 to
dcb6235
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a memory reservation issue when sorting TVList data structures during query execution. The main problem occurs when an unordered TVList is cloned for a new query - while the original data memory is already reserved, the indices array created during sorting requires additional memory that wasn't initially accounted for.
Key Changes
- Added memory reservation logic in TVList.set() to reserve memory for indices arrays when they are created during sorting
- Added getReservedMemory() method to MemoryReservationManager interface and all its implementations to support testing
- Added comprehensive test case to validate memory reservation correctness when querying and sorting TVList
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| TVList.java | Added logic to reserve memory for indices arrays when created during sorting operations |
| MemoryReservationManager.java | Added getReservedMemory() interface method marked as TestOnly |
| NotThreadSafeMemoryReservationManager.java | Implemented getReservedMemory() to calculate total reserved memory |
| ThreadSafeMemoryReservationManager.java | Implemented thread-safe getReservedMemory() method |
| FakedMemoryReservationManager.java | Implemented no-op getReservedMemory() returning 0 |
| FragmentInstanceExecutionTest.java | Added test case and helper method to validate memory reservation during query execution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| IMemTable memTable = new PrimitiveMemTable("root.test", "1"); | ||
|
|
||
| int rows = 100; | ||
| for (int i = 0; i < 100; i++) { |
There was a problem hiding this comment.
The variable rows is declared with the value 100, but it's not actually used in the loop logic. The loop iterates from 0 to 99, and the timestamp is calculated as rows - i - 1, but this could be simplified to just use the literal 100 or properly use the rows variable in the loop condition.
| for (int i = 0; i < 100; i++) { | |
| for (int i = 0; i < rows; i++) { |
| ReadOnlyMemChunk readOnlyMemChunk2 = | ||
| memTable.query(fragmentInstanceContext2, fullPath, Long.MIN_VALUE, null, null); |
There was a problem hiding this comment.
The test creates two ReadOnlyMemChunk instances (readOnlyMemChunk1 and readOnlyMemChunk2) but only uses readOnlyMemChunk1. The unused variable readOnlyMemChunk2 should either be used in the test logic or removed to improve code clarity.
| ReadOnlyMemChunk readOnlyMemChunk2 = | |
| memTable.query(fragmentInstanceContext2, fullPath, Long.MIN_VALUE, null, null); |
| } | ||
| // Reserve memory for indices if the TVList is owned by a query | ||
| if (ownerQuery != null) { | ||
| long indicesBytes = indices.size() * PrimitiveArrayManager.ARRAY_SIZE * 4L; |
There was a problem hiding this comment.
Potential overflow in int multiplication before it is converted to long by use in a numeric context.
| long indicesBytes = indices.size() * PrimitiveArrayManager.ARRAY_SIZE * 4L; | |
| long indicesBytes = | |
| (long) indices.size() * PrimitiveArrayManager.ARRAY_SIZE * Integer.BYTES; |
| // Reserve memory for indices if the TVList is owned by a query | ||
| if (ownerQuery != null) { | ||
| long indicesBytes = indices.size() * PrimitiveArrayManager.ARRAY_SIZE * 4L; | ||
| MemoryReservationManager memoryReservationManager = | ||
| ((FragmentInstanceContext) ownerQuery).getMemoryReservationContext(); | ||
| memoryReservationManager.reserveMemoryCumulatively(indicesBytes); | ||
| } |
There was a problem hiding this comment.
- if we need to lock this tvlist?
- what if reserveMemoryCumulatively failed?(especially sort is called by flush other than query)
2 participants
When the TVList is unordered and a query already exists, the new query will clone this TVList. The memory occupied by the original TVList is released by the previous query. However, when sorting the TVList, it creates an indices array, so we need to reserve the additional memory.