Copilot commented on code in PR #61973:
URL: https://github.com/apache/doris/pull/61973#discussion_r3019033254
##########
be/test/exec/operator/partitioned_aggregation_source_operator_test.cpp:
##########
@@ -737,6 +737,44 @@ TEST_F(PartitionedAggregationSourceOperatorTest,
RevocableMemSizeWithAggContaine
EXPECT_EQ(source_operator->revocable_mem_size(_helper.runtime_state.get()),
container_bytes);
}
+// Condition: partition level >= _repartition_max_depth → 0 (not revocable).
+TEST_F(PartitionedAggregationSourceOperatorTest,
RevocableMemSizeAtMaxDepthReturnsZero) {
+ auto [source_operator, sink_operator] = _helper.create_operators();
+ const auto tnode = _helper.create_test_plan_node();
+ ASSERT_TRUE(source_operator->init(tnode,
_helper.runtime_state.get()).ok());
+ ASSERT_TRUE(source_operator->prepare(_helper.runtime_state.get()).ok());
+
+ std::shared_ptr<MockPartitionedAggSharedState> shared_state;
+ auto* local_state =
_helper.create_source_local_state(_helper.runtime_state.get(),
+
source_operator.get(), shared_state);
+
+ // Add a large block so that without the depth check, revocable_mem_size
would be > 0.
+ std::vector<int32_t> large_data(300000);
+ std::iota(large_data.begin(), large_data.end(), 0);
+ auto large_block = ColumnHelper::create_block<DataTypeInt32>(large_data);
+ ASSERT_GT(large_block.allocated_bytes(), 1UL << 20);
+ local_state->_blocks.push_back(std::move(large_block));
+
+ SpillFileSPtr spill_file;
+ ASSERT_TRUE(ExecEnv::GetInstance()
+ ->spill_file_mgr()
+ ->create_spill_file("ut/revocable_max_depth",
spill_file)
+ .ok());
+ local_state->_current_partition.spill_file = spill_file;
+
+ ASSERT_GE(source_operator->_repartition_max_depth, 2);
+
+ // Set partition level to max depth → not revocable.
+ local_state->_current_partition.level =
+ static_cast<int>(source_operator->_repartition_max_depth) - 1;
+
EXPECT_EQ(source_operator->revocable_mem_size(_helper.runtime_state.get()),
0UL);
+
+ // Also verify level == max_depth - 1 IS still revocable.
Review Comment:
This comment says `level == max_depth - 1` is still revocable, but the code
below sets the level to `max_depth - 2` for the revocable case. Please correct
the comment so it matches the asserted behavior.
```suggestion
// Also verify level == max_depth - 2 (one below max depth) IS still
revocable.
```
##########
be/test/exec/operator/partitioned_hash_join_probe_operator_test.cpp:
##########
@@ -1027,6 +1027,51 @@ TEST_F(PartitionedHashJoinProbeOperatorTest,
revocable_mem_size) {
ASSERT_EQ(probe_operator->revocable_mem_size(_helper.runtime_state.get()),
0);
}
+// Partition (level + 1) >= _repartition_max_depth → revocable_mem_size
returns 0.
+TEST_F(PartitionedHashJoinProbeOperatorTest, revocable_mem_size_at_max_depth) {
+ auto [probe_operator, sink_operator] = _helper.create_operators();
+
+ std::shared_ptr<MockPartitionedHashJoinSharedState> shared_state;
+ auto local_state =
_helper.create_probe_local_state(_helper.runtime_state.get(),
+ probe_operator.get(),
shared_state);
+
+ local_state->_shared_state->_is_spilled = true;
+ local_state->_child_eos = true;
+
+ // Set up a valid current partition with build not finished (the path that
+ // checks level vs max depth).
+ SpillFileSPtr build_file;
+ ASSERT_TRUE(ExecEnv::GetInstance()
+ ->spill_file_mgr()
+
->create_spill_file("ut/revocable_join_max_depth_build", build_file)
+ .ok());
+ SpillFileSPtr probe_file;
+ ASSERT_TRUE(ExecEnv::GetInstance()
+ ->spill_file_mgr()
+
->create_spill_file("ut/revocable_join_max_depth_probe", probe_file)
+ .ok());
Review Comment:
These spill file paths are hard-coded. In this test file, most spill files
use a query_id/next_id-based relative path to avoid collisions across runs or
parallel test execution. Consider generating unique paths here as well to
prevent flaky failures if the same path already exists.
##########
be/test/exec/operator/partitioned_aggregation_source_operator_test.cpp:
##########
@@ -737,6 +737,44 @@ TEST_F(PartitionedAggregationSourceOperatorTest,
RevocableMemSizeWithAggContaine
EXPECT_EQ(source_operator->revocable_mem_size(_helper.runtime_state.get()),
container_bytes);
}
+// Condition: partition level >= _repartition_max_depth → 0 (not revocable).
+TEST_F(PartitionedAggregationSourceOperatorTest,
RevocableMemSizeAtMaxDepthReturnsZero) {
Review Comment:
The test comment says `partition level >= _repartition_max_depth` implies
not revocable, but the implementation checks `(level + 1) >=
_repartition_max_depth` (i.e., `level >= max_depth - 1`). Please update the
comment to match the actual boundary condition to avoid off-by-one confusion.
--
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]
