fix: Change count metric from signed to unsigned (int64_t -> uint64_t)#27276
Closed
peterenescu wants to merge 1 commit into
Closed
fix: Change count metric from signed to unsigned (int64_t -> uint64_t)#27276peterenescu wants to merge 1 commit into
peterenescu wants to merge 1 commit into
Conversation
Summary: Use an unsigned type for the count metric while keeping int64_t for value metrics. When the unit is kNone, negative values can be valid such as for delta- type metrics, so int64_t remains appropriate. In contrast, `count` should always be non-negative. This PR also addresses potential overflow when converting unsigned metrics to `RuntimeMetrics`. X-link: facebookincubator/velox#15536 Reviewed By: Yuhta Differential Revision: D95451420 Pulled By: peterenescu
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideChanges the Presto runtime metric conversion to treat the metric count as an unsigned value and safely cast it when building a RuntimeMetric, preventing overflow issues. Class diagram for updated metric count handlingclassDiagram
class Metric {
<<struct>>
RuntimeUnit unit
int64_t sum
uint64_t count
int64_t max
int64_t min
}
class RuntimeMetric {
<<struct>>
RuntimeUnit unit
int64_t sum
int64_t count
int64_t max
int64_t min
}
class PrestoTask {
+protocol_RuntimeMetric toRuntimeMetric(string name, Metric metric)
}
class RuntimeMetricsUtils {
+int64_t saturateCast(uint64_t value)
+protocol_RuntimeUnit toPrestoRuntimeUnit(RuntimeUnit unit)
}
PrestoTask --> Metric : uses
PrestoTask --> RuntimeMetric : builds
PrestoTask ..> RuntimeMetricsUtils : calls
Metric --> RuntimeMetric : converted_via_toRuntimeMetric
RuntimeMetricsUtils <.. RuntimeMetric : safe_cast_for_count
Flow diagram for safe casting of metric count to RuntimeMetricflowchart LR
A["Metric.count (uint64_t)"] --> B["saturateCast(count)"]
B --> C["RuntimeMetric.count (int64_t)"]
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 left some high level feedback:
- If
saturateCastis defined in a more specific header thanRuntimeMetrics.h, consider including that directly instead to avoid an unnecessary dependency on the runtime metrics header fromPrestoTask.cpp.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- If `saturateCast` is defined in a more specific header than `RuntimeMetrics.h`, consider including that directly instead to avoid an unnecessary dependency on the runtime metrics header from `PrestoTask.cpp`.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Hi @peterenescu, sorry if I misunderstood something earlier. Based on the #27295 (comment), I assumed that the correct fix is the one implemented in this PR. Would you like to reopen this PR so we can proceed with the fix in Presto, or would you prefer that I follow up with the changes? Thanks! |
7 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:
Use an unsigned type for the count metric while keeping int64_t for value
metrics. When the unit is kNone, negative values can be valid such as for delta-
type metrics, so int64_t remains appropriate. In contrast,
countshouldalways be non-negative. This PR also addresses potential overflow when
converting unsigned metrics to
RuntimeMetrics.X-link: facebookincubator/velox#15536
Reviewed By: Yuhta
Differential Revision: D95451420
Pulled By: peterenescu
Summary by Sourcery
Bug Fixes: