[presto-native-execution] Expose spill_io_stats_key_prefix as a session property (#27694)#27694
[presto-native-execution] Expose spill_io_stats_key_prefix as a session property (#27694)#27694mkarrmann wants to merge 1 commit intoprestodb:masterfrom
Conversation
Reviewer's GuideAdds a new native execution session property to control the suffix used for spill FileSystem IoStats keys, wiring it through session properties to QueryConfig so operators can disambiguate spill IoStats from connector IoStats in shared FileSystem deployments. Class diagram for new spill IoStats suffix session propertyclassDiagram
class SessionProperties {
+SessionProperties()
+addSessionProperty(name, description, type, isHidden, configKey, defaultValue)
<<static>> const char* kSpillFileCreateConfig
<<static>> const char* kSpillIoStatsKeySuffix
<<static>> const char* kAggregationSpillFileCreateConfig
}
class QueryConfig {
<<static>> const char* kSpillFileCreateConfig
<<static>> const char* kSpillIoStatsKeySuffix
+spillFileCreateConfig()
+spillIoStatsKeySuffix()
}
SessionProperties ..> QueryConfig : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The descriptive text for
kSpillIoStatsKeySuffixinSessionProperties.cppand the header comment inSessionProperties.hare slightly divergent (e.g., mentioningruntimeStatsvs.TableScan/TableWritercollisions); consider consolidating or aligning these to avoid future confusion about the primary purpose of the property. - Given this property is intended to avoid silent IoStats key collisions, consider explicitly documenting in the description what happens when the suffix is left empty in shared FileSystem deployments (e.g., collisions remain possible), so users understand the tradeoff of using the default.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The descriptive text for `kSpillIoStatsKeySuffix` in `SessionProperties.cpp` and the header comment in `SessionProperties.h` are slightly divergent (e.g., mentioning `runtimeStats` vs. `TableScan/TableWriter` collisions); consider consolidating or aligning these to avoid future confusion about the primary purpose of the property.
- Given this property is intended to avoid silent IoStats key collisions, consider explicitly documenting in the description what happens when the suffix is left empty in shared FileSystem deployments (e.g., collisions remain possible), so users understand the tradeoff of using the default.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…on property (prestodb#27694) Summary: Adds Presto session property in order to set the Spill IO stats suffix introduced in facebookincubator/velox#17390 Differential Revision: D103308964
04a51da to
abfa23a
Compare
|
Summary:
Adds Presto session property in order to set the Spill IO stats suffix introduced in facebookincubator/velox#17390
Differential Revision: D103308964