Minor: Remove memory reservation in JoinLeftData used in HashJoin
#13751
Conversation
… field in hash join implementation
…atchState enums This commit enhances the HashJoinStreamState and ProcessProbeBatchState structures by implementing the Debug and Clone traits, allowing for easier debugging and cloning of these state representations in the hash join implementation.
|
Good catch. Some day ago it was intention to add spilling to HJ #12952 |
| @@ -90,9 +90,6 @@ struct JoinLeftData { | |||
| /// Counter of running probe-threads, potentially | |||
| /// able to update `visited_indices_bitmap` | |||
| probe_threads_counter: AtomicUsize, | |||
| /// Memory reservation that tracks memory used by `hash_map` hash table | |||
| /// `batch`. Cleared on drop. | |||
| _reservation: MemoryReservation, | |||
There was a problem hiding this comment.
In case reservation is not stored in JoinLeftData, it'll be dropped right after collect_left_input due to being unneeded anymore. And due to MemoryReservation calls free on its drop, it looks like that with this patch, during the whole join execution build side data will be untracked by memory pool.
So the initial idea of this attribute was to make reservation to live as long as JoinLeftData exists.
There was a problem hiding this comment.
UPD: some "logs" (additional prints 😞 ) just to check join execution before/after this patch:
// Before
---- joins::hash_join::tests::join_inner_one_no_shared_column_names stdout ----
Waiting build side
Reserving 324 bytes
Reserving 124 bytes
Fetching probe batch
Processing probe batch
Fetching probe batch
Processing unmatched build side
Freeing 448 bytes
// After
---- joins::hash_join::tests::join_inner_one_no_shared_column_names stdout ----
Waiting build side
Reserving 324 bytes
Reserving 124 bytes
Freeing 448 bytes
Fetching probe batch
Processing probe batch
Fetching probe batch
Processing unmatched build side
so yes, the memory is "freed" before join completes its execution, which doesn't seem to be an expected behavior.
There was a problem hiding this comment.
The reason I think this is not an issue is because it only matters when we run 'collect_left_join', we track the memory and free after the build side is done. Although, the memory is clean while the hash map is not freed. BUT, since we don't have expected memory change in the probe stage, the memory is not helpful for the probe stage. That's why I think this will not cause any issues if we drop the reservation early.
There was a problem hiding this comment.
Although, the memory is clean while the hash map is not freed
That actually is the issue, because there is single memory pool for a RuntimeEnv, and it's intended to be used by multiple sessions or at least by the multiple operators within the same query. Even in case of single query like select * from a join b order by a.field untracked build side may cause OOM due to SortExec will consider MemoryPool as being empty, while there is a build side data in memory.
Which issue does this PR close?
Closes #.
Rationale for this change
memory reservation is not used outside of
collect_left_input, I don't think there is any good reason to add it in JoinLeftData. Remove this so we don't need to create dummy value for reservation when we create JoinLeftDataWhat changes are included in this PR?
Are these changes tested?
Also, Debug and Clone for
HashJoinStreamStateAre there any user-facing changes?