github-actions[bot] commented on code in PR #61973:
URL: https://github.com/apache/doris/pull/61973#discussion_r3018899476
##########
be/src/exec/operator/partitioned_hash_join_probe_operator.cpp:
##########
@@ -875,6 +875,11 @@ size_t
PartitionedHashJoinProbeOperatorX::revocable_mem_size(RuntimeState* state
// Or if current partition has finished build hash table.
return 0;
}
+ // If the current partition has reached the max repartition depth, it
cannot be
+ // repartitioned further, so its data is not revocable.
+ if (local_state._current_partition.level >= _repartition_max_depth) {
Review Comment:
This has the same off-by-one as the aggregation path. `revoke_memory()` here
goes through `repartition_current_partition()`, which errors once
`partition.level + 1 >= _repartition_max_depth`. That means `level ==
_repartition_max_depth - 1` is already non-revocable, but this guard still
reports the recovered build block as revocable and can drive the query into
`InternalError`. Please align the cutoff with the actual repartition failure
boundary, and update the new UT to exercise `max_depth - 1` as the zero case.
```suggestion
if (local_state._current_partition.level >= _repartition_max_depth - 1) {
return 0;
}
```
##########
be/src/exec/operator/partitioned_aggregation_source_operator.cpp:
##########
@@ -191,6 +191,11 @@ size_t
PartitionedAggSourceOperatorX::revocable_mem_size(RuntimeState* state) co
if (!local_state._shared_state->_is_spilled ||
!local_state._current_partition.spill_file) {
return 0;
}
+ // If the current partition has reached the max repartition depth, it
cannot be
+ // repartitioned further, so its data is not revocable.
+ if (local_state._current_partition.level >= _repartition_max_depth) {
Review Comment:
This guard is one level too late. `revoke_memory()` eventually calls
`_flush_and_repartition()`, which computes `new_level = current_level + 1` and
fails once `new_level >= _repartition_max_depth`. So a partition at `level ==
_repartition_max_depth - 1` is already non-revocable: reporting it here lets
the scheduler try to revoke it and hit `InternalError` instead of finishing the
partition in memory. The cutoff needs to match that failure boundary, and the
new UT should assert on `max_depth - 1` rather than `max_depth`.
```suggestion
if (local_state._current_partition.level >= _repartition_max_depth - 1) {
return 0;
}
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
