feat: Add session property aggregation_memory_compaction_reclaim_enabled#27221
Merged
duxiao1212 merged 1 commit intoprestodb:masterfrom Mar 4, 2026
Merged
feat: Add session property aggregation_memory_compaction_reclaim_enabled#27221duxiao1212 merged 1 commit intoprestodb:masterfrom
duxiao1212 merged 1 commit intoprestodb:masterfrom
Conversation
Contributor
Reviewer's GuideAdds a new session property to control whether lightweight aggregation memory compaction is attempted during memory reclaim before spilling, wiring it through native Presto C++ session properties, Java native worker session properties, query config mapping tests, and updating related constants and documentation scaffolding. Class diagram for new aggregation_memory_compaction_reclaim_enabled session property wiringclassDiagram
class SessionProperties {
+static const char* kAggregationCompactionUnusedMemoryRatio
+static const char* kAggregationMemoryCompactionReclaimEnabled
+SessionProperties()
+std::string toVeloxConfig(const std::string& key)
+bool hasVeloxConfig(const std::string& key)
}
class QueryConfig {
+static const char* kAggregationCompactionUnusedMemoryRatio
+static const char* kAggregationMemoryCompactionReclaimEnabled
+double aggregationCompactionUnusedMemoryRatio()
+bool aggregationMemoryCompactionReclaimEnabled()
}
class NativeWorkerSessionPropertyProvider {
+static final String NATIVE_AGGREGATION_COMPACTION_BYTES_THRESHOLD
+static final String NATIVE_AGGREGATION_COMPACTION_UNUSED_MEMORY_RATIO
+static final String NATIVE_AGGREGATION_MEMORY_COMPACTION_RECLAIM_ENABLED
+NativeWorkerSessionPropertyProvider(FeaturesConfig featuresConfig)
}
class FeaturesConfig {
+boolean isNativeExecution()
}
SessionProperties --> QueryConfig : uses
NativeWorkerSessionPropertyProvider --> FeaturesConfig : uses
NativeWorkerSessionPropertyProvider ..> SessionProperties : aligns_property_keys
Flow diagram for aggregation_memory_compaction_reclaim_enabled behavior during memory reclaimflowchart TD
A["Start memory reclaim in aggregation operator"] --> B{"aggregation_memory_compaction_reclaim_enabled?"}
B -- "true" --> C["Perform lightweight memory compaction\n(e.g., free dead aggregate state)"]
C --> D{"Is further reclaim needed?"}
D -- "yes" --> E["Spill aggregation state to disk"]
D -- "no" --> F["Continue in-memory aggregation"]
B -- "false" --> E
E --> G["Resume query execution after spilling"]
F --> G
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="presto-native-execution/presto_cpp/main/tests/SessionPropertiesTest.cpp" line_range="137-138" />
<code_context>
core::QueryConfig::kAggregationCompactionBytesThreshold},
{SessionProperties::kAggregationCompactionUnusedMemoryRatio,
core::QueryConfig::kAggregationCompactionUnusedMemoryRatio},
+ {SessionProperties::kAggregationMemoryCompactionReclaimEnabled,
+ core::QueryConfig::kAggregationMemoryCompactionReclaimEnabled},
{SessionProperties::kMergeJoinOutputBatchStartSize,
core::QueryConfig::kMergeJoinOutputBatchStartSize}};
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests that cover default and overridden values for the new session property, not just the mapping key.
Right now this only verifies the wiring between the session property name and the `QueryConfig` key. Please also add tests in this file that (1) check the default value of `aggregationMemoryCompactionReclaimEnabled` from `QueryConfig` is propagated through `SessionProperties::toVeloxConfig`, and (2) confirm that explicitly setting the session property to true/false results in the corresponding Velox config value. This will validate the full config → session property → Velox config path for this flag.
Suggested implementation:
```cpp
{SessionProperties::kAggregationCompactionUnusedMemoryRatio,
core::QueryConfig::kAggregationCompactionUnusedMemoryRatio},
{SessionProperties::kAggregationMemoryCompactionReclaimEnabled,
core::QueryConfig::kAggregationMemoryCompactionReclaimEnabled},
{SessionProperties::kMergeJoinOutputBatchStartSize,
core::QueryConfig::kMergeJoinOutputBatchStartSize}};
// Verify the default value from QueryConfig is propagated when the session
// property is not set.
TEST(SessionPropertiesTest,
AggregationMemoryCompactionReclaimEnabled_DefaultPropagation) {
// Empty session properties => use QueryConfig defaults.
std::unordered_map<std::string, std::string> sessionProperties;
// Construct QueryConfig with defaults.
core::QueryConfig queryConfig{{}};
// Build Velox config from session properties + query config.
velox::Config veloxConfig;
SessionProperties::toVeloxConfig(sessionProperties, queryConfig, veloxConfig);
// The Velox config value should match the QueryConfig default.
EXPECT_EQ(
veloxConfig.getBool(
core::QueryConfig::kAggregationMemoryCompactionReclaimEnabled),
queryConfig.aggregationMemoryCompactionReclaimEnabled());
}
// Verify that explicitly setting the session property to true/false is
// reflected in the resulting Velox config value.
TEST(SessionPropertiesTest,
AggregationMemoryCompactionReclaimEnabled_OverrideViaSessionProperty) {
core::QueryConfig queryConfig{{}};
{
// Override to true.
std::unordered_map<std::string, std::string> sessionProperties;
sessionProperties.emplace(
SessionProperties::kAggregationMemoryCompactionReclaimEnabled, "true");
velox::Config veloxConfig;
SessionProperties::toVeloxConfig(
sessionProperties, queryConfig, veloxConfig);
EXPECT_TRUE(veloxConfig.getBool(
core::QueryConfig::kAggregationMemoryCompactionReclaimEnabled));
}
{
// Override to false.
std::unordered_map<std::string, std::string> sessionProperties;
sessionProperties.emplace(
SessionProperties::kAggregationMemoryCompactionReclaimEnabled, "false");
velox::Config veloxConfig;
SessionProperties::toVeloxConfig(
sessionProperties, queryConfig, veloxConfig);
EXPECT_FALSE(veloxConfig.getBool(
core::QueryConfig::kAggregationMemoryCompactionReclaimEnabled));
}
}
```
These edits assume the following, based on existing conventions in similar tests:
1. `SessionProperties::toVeloxConfig` has a signature compatible with:
`static void toVeloxConfig(const std::unordered_map<std::string, std::string>&, const core::QueryConfig&, velox::Config&);`
2. `velox::Config` exposes `getBool(const std::string&)` (or a compatible API) and is already included in this test file.
3. `core::QueryConfig` has a method `aggregationMemoryCompactionReclaimEnabled()` returning the typed default for the new flag, and the config map constructor `core::QueryConfig{{}}` (or similar) is valid in this file as used for other tests.
If the existing tests in this file use different container types for session properties, a different Velox config type, or a helper method to build the config (for example a `makeVeloxConfig(...)` helper), you should:
- Replace `std::unordered_map<std::string, std::string>` with the same type used elsewhere (e.g. `facebook::presto::protocol::SessionProperties` or a typedef).
- Replace `velox::Config` and `getBool(...)` with the actual config type and getter used by the other tests (for example `velox::core::Config` and `get<bool>(...)`).
- Construct `core::QueryConfig` using the same initialization pattern already used in this file so that the default value of `aggregationMemoryCompactionReclaimEnabled` is not overridden (e.g. via a helper like `createQueryConfig()` if one exists).
Keep the three expectations identical in spirit:
1. No session property set → Velox config value equals `QueryConfig` default.
2. Session property set to `"true"` → Velox config flag is true.
3. Session property set to `"false"` → Velox config flag is false.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
steveburnett
requested changes
Feb 26, 2026
Contributor
steveburnett
left a comment
There was a problem hiding this comment.
Thank you for the documentation! Just one nit of phrasing.
xiaoxmeng
previously approved these changes
Feb 26, 2026
Contributor
xiaoxmeng
left a comment
There was a problem hiding this comment.
@duxiao1212 please address comments before land. Thanks!
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Feb 26, 2026
…led (prestodb#27221) Summary: Per title Differential Revision: D94130609
d49679b to
cfac73c
Compare
steveburnett
approved these changes
Feb 26, 2026
Contributor
steveburnett
left a comment
There was a problem hiding this comment.
LGTM! (docs)
Pull updated branch, new local doc build, looks good. Thanks!
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Mar 1, 2026
…led (prestodb#27221) Summary: Per title Reviewed By: xiaoxmeng Differential Revision: D94130609
cfac73c to
1e05c25
Compare
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Mar 1, 2026
…led (prestodb#27221) Summary: Per title Reviewed By: xiaoxmeng Differential Revision: D94130609
1e05c25 to
e983ce6
Compare
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Mar 1, 2026
…led (prestodb#27221) Summary: Pull Request resolved: prestodb#27221 Per title Reviewed By: xiaoxmeng Differential Revision: D94130609
e983ce6 to
acbc97c
Compare
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Mar 1, 2026
…led (prestodb#27221) Summary: Per title Reviewed By: xiaoxmeng Differential Revision: D94130609
acbc97c to
42df7b0
Compare
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Mar 1, 2026
…led (prestodb#27221) Summary: Per title Reviewed By: xiaoxmeng Differential Revision: D94130609
42df7b0 to
39c7c7e
Compare
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Mar 2, 2026
…led (prestodb#27221) Summary: Per title Reviewed By: xiaoxmeng Differential Revision: D94130609
39c7c7e to
00c1a39
Compare
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Mar 2, 2026
…led (prestodb#27221) Summary: Per title Reviewed By: xiaoxmeng Differential Revision: D94130609
00c1a39 to
c40b7eb
Compare
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Mar 2, 2026
…led (prestodb#27221) Summary: Pull Request resolved: prestodb#27221 Per title Reviewed By: xiaoxmeng Differential Revision: D94130609
c40b7eb to
97a57cc
Compare
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Mar 2, 2026
…led (prestodb#27221) Summary: Per title Reviewed By: xiaoxmeng Differential Revision: D94130609
97a57cc to
7311731
Compare
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Mar 2, 2026
…led (prestodb#27221) Summary: Per title Reviewed By: xiaoxmeng Differential Revision: D94130609
7311731 to
0f73364
Compare
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Mar 3, 2026
…led (prestodb#27221) Summary: Per title Reviewed By: xiaoxmeng Differential Revision: D94130609
0f73364 to
1bd0481
Compare
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Mar 3, 2026
…led (prestodb#27221) Summary: Pull Request resolved: prestodb#27221 Per title Reviewed By: xiaoxmeng Differential Revision: D94130609
1bd0481 to
ed89c99
Compare
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Mar 3, 2026
…led (prestodb#27221) Summary: Per title Reviewed By: xiaoxmeng Differential Revision: D94130609
ed89c99 to
4ec3eaf
Compare
…led (prestodb#27221) Summary: Per title Reviewed By: xiaoxmeng Differential Revision: D94130609
4ec3eaf to
84c7c08
Compare
NikhilCollooru
approved these changes
Mar 4, 2026
This was referenced Mar 31, 2026
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary: Per title
Differential Revision: D94130609
Summary by Sourcery
Add a configurable session property to control lightweight aggregation memory compaction during memory reclaim before spilling.
New Features:
Tests: