fix: Change count metric from signed to unsigned (int64_t -> uint64_t) by rui-mo · Pull Request #15536 · facebookincubator/velox

rui-mo · 2025-11-18T07:05:01Z

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.

netlify · 2025-11-18T07:05:07Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`1bfaa5e`
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/69b08f5881004e0008c51819

majetideepak · 2025-11-18T14:32:06Z

@rui-mo I see a test failing [ FAILED ] RuntimeMetricsTest.basic (0 ms).

@tanjialiang, @xiaoxmeng Do you have any thoughts on this PR?

rui-mo · 2025-11-18T14:38:02Z

Thanks @majetideepak. The test should be fixed now.

tanjialiang

Thanks @rui-mo for the improvements. Could you make sure the types aligns also for the calling methods and the counters' assignings? Unexpected overflows might happen if not well aligned for this type of downcast.

tanjialiang · 2025-11-24T18:37:58Z

velox/common/base/tests/RuntimeMetricsTest.cpp

+      int64_t expectedMin = std::numeric_limits<uint64_t>::max(),
+      int64_t expectedMax = std::numeric_limits<uint64_t>::min()) {


assigning a uint64 max to int64

Thanks for catching this. I double-checked those to ensure unexpected overflow would not occur.

tanjialiang · 2025-12-02T22:45:00Z

It just came to my mind: Should we keep value signed? When Unit is kNone, we might expect legit negative value metrics such as delta type of metrics.

counts should be non-negative this should be strictly true.

rui-mo · 2025-12-03T14:20:50Z

@tanjialiang If it’s necessary to keep the value as int64_t, I’m OK with doing so. In that case, we’ll need special handling for range checks when passing uint64_t metrics to RuntimeMetrics. For example, IoCounter uses uint64_t, which could overflow an int64_t.

tanjialiang · 2025-12-04T17:59:58Z

@tanjialiang If it’s necessary to keep the value as int64_t, I’m OK with doing so. In that case, we’ll need special handling for range checks when passing uint64_t metrics to RuntimeMetrics. For example, IoCounter uses uint64_t, which could overflow an int64_t.

For most cases, we should be good. But yeah for the ones we already know that is doing a uint64 to int64 assignment we can add impose range checks. (and it should be okay if we miss some. most cases they won't overflow)

rui-mo · 2025-12-10T14:06:56Z

@tanjialiang I’ve updated the PR based on your suggestion. uint64_t is now used only for count, and additional range checks have been added to prevent potential overflow. Please take another look when you have a moment. Thanks!

karthikeyann

approving for velox-cudf changes.

rui-mo · 2026-02-03T10:21:36Z

Hi @tanjialiang, could you please take another look at your convenience? Thanks.

rui-mo · 2026-03-02T16:43:27Z

Hi @Yuhta @tanjialiang, this PR fixes the overflow issue after propagating IO stats to runtime metrics, see #15408 (comment). Could you please take further review? Thanks.

Yuhta · 2026-03-03T17:50:45Z

velox/experimental/cudf/exec/CudfHashAggregation.cpp

@@ -1159,7 +1159,7 @@ CudfVectorPtr CudfHashAggregation::releaseAndResetPartialOutput() {
        std::string(exec::HashAggregation::kFlushTimes), RuntimeCounter(1));
    lockedStats->addRuntimeStat(
        std::string(exec::HashAggregation::kPartialAggregationPct),
-        RuntimeCounter(aggregationPct));
+        RuntimeCounter(static_cast<int64_t>(aggregationPct)));


Should this one be saturate cast as well?

I assumed aggregationPct would typically be <= 100, so I used static_cast. I've updated it to use a saturate cast to prevent any potential risk. Thanks.

meta-codesync · 2026-03-05T22:52:09Z

@peterenescu has imported this pull request. If you are a Meta employee, you can view this in D95451420.

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

peterenescu · 2026-03-06T18:35:29Z

Hi @rui-mo you will also need to make a small change to presto prestodb/presto#27276. We will probably need to:

Add compatibility layer
Merge the Velox code
Update Presto side
Remove the compatibility layer in Velox
cc: @Yuhta

rui-mo · 2026-03-06T20:43:10Z

@peterenescu Thanks for your help. I understand - that’s because RuntimeMetric in Presto also uses int64_t for count.

namespace facebook::presto::protocol {
struct RuntimeMetric {
  String name = {};
  RuntimeUnit unit = {};
  int64_t sum = {};
  int64_t count = {};
  int64_t max = {};
  int64_t min = {};
};

rui-mo · 2026-03-06T20:46:35Z

@peterenescu Could you please let me know what I need to do to "add compatibility layer" in Velox?

Yuhta · 2026-03-06T21:32:37Z

@rui-mo Maybe we change the presto side first to be uint64_t? Will this error go away?

rui-mo · 2026-03-10T21:52:55Z

Hi @peterenescu @Yuhta, based on the review comment from prestodb/presto#27295 (comment), it seems that we could merge this PR into Velox first and apply the corresponding changes in Presto afterward. I would appreciate your thoughts on this approach. Thank you!

Yuhta · 2026-03-10T22:44:36Z

@rui-mo The merge into Meta internal codebase is blocked unless the Presto side is fixed first. If the changed code on Presto side works with both before & after with Velox, we should change Presto first; otherwise we may need to use VELOX_ENABLE_BACKWARD_COMPATIBILITY

## Description  Presto's RuntimeMetric uses int64_t for count, but Velox's RuntimeMetric uses uint64_t. To avoid overflow, we cap the count at int64_t's max value. ## Motivation and Context   A refactor to change the count metric from int64_t to uint64_t is going on in Velox. To avoid overflow when converting to Presto metric, this PR updates Presto count metric to consistent type. facebookincubator/velox#15536 ``` == NO RELEASE NOTE == ```

…7295) ## Description  Presto's RuntimeMetric uses int64_t for count, but Velox's RuntimeMetric uses uint64_t. To avoid overflow, we cap the count at int64_t's max value. ## Motivation and Context   A refactor to change the count metric from int64_t to uint64_t is going on in Velox. To avoid overflow when converting to Presto metric, this PR updates Presto count metric to consistent type. facebookincubator/velox#15536 ``` == NO RELEASE NOTE == ```

meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 18, 2025

rui-mo changed the title ~~fix: Use uint64_t in the RuntimeMetric~~ fix: Change RuntimeMetrics counters from signed to unsigned (int64_t -> uint64_t) Nov 18, 2025

rui-mo mentioned this pull request Nov 18, 2025

misc: Propagate full IO stats (count/min/max) to runtime metrics #15408

Closed

majetideepak requested review from Yuhta, kevinwilfong, spershin, tanjialiang and xiaoxmeng November 18, 2025 14:29

rui-mo force-pushed the wip_uint64 branch from 4ba4543 to 7e2d88f Compare November 18, 2025 14:36

tanjialiang reviewed Nov 24, 2025

View reviewed changes

rui-mo force-pushed the wip_uint64 branch from 7e2d88f to d9faa10 Compare November 25, 2025 15:00

rui-mo requested review from bdice, devavret, karthikeyann, majetideepak and mhaseeb123 as code owners November 25, 2025 15:00

rui-mo force-pushed the wip_uint64 branch 2 times, most recently from 573291a to f5ddb61 Compare December 2, 2025 06:12

rui-mo force-pushed the wip_uint64 branch from f5ddb61 to 3d9869f Compare December 10, 2025 06:56

rui-mo changed the title ~~fix: Change RuntimeMetrics counters from signed to unsigned (int64_t -> uint64_t)~~ fix: Change RuntimeMetrics count from signed to unsigned (int64_t -> uint64_t) Dec 10, 2025

rui-mo force-pushed the wip_uint64 branch from 3d9869f to d65c1dd Compare December 10, 2025 07:11

rui-mo changed the title ~~fix: Change RuntimeMetrics count from signed to unsigned (int64_t -> uint64_t)~~ fix: Change count metric from signed to unsigned (int64_t -> uint64_t) Dec 10, 2025

rui-mo force-pushed the wip_uint64 branch from d65c1dd to a2bb115 Compare December 18, 2025 13:06

karthikeyann approved these changes Feb 2, 2026

View reviewed changes

rui-mo force-pushed the wip_uint64 branch from bf8b5c4 to 6f4df44 Compare February 3, 2026 10:19

rui-mo force-pushed the wip_uint64 branch from 6f4df44 to 6aa1f9c Compare March 2, 2026 16:39

rui-mo force-pushed the wip_uint64 branch from 6aa1f9c to 1d29847 Compare March 3, 2026 12:03

Yuhta reviewed Mar 3, 2026

View reviewed changes

rui-mo force-pushed the wip_uint64 branch 4 times, most recently from 2e4cbb2 to 0879e2e Compare March 5, 2026 15:10

Yuhta approved these changes Mar 5, 2026

View reviewed changes

Yuhta added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Mar 5, 2026

peterenescu mentioned this pull request Mar 6, 2026

fix: Change count metric from signed to unsigned (int64_t -> uint64_t) prestodb/presto#27276

Closed

peterenescu removed the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Mar 6, 2026

rui-mo mentioned this pull request Mar 9, 2026

fix(native): Caps the count metric at int64_t's max value prestodb/presto#27295

Merged

7 tasks

Use uint64_t for count

1bfaa5e

rui-mo force-pushed the wip_uint64 branch from 0879e2e to 1bfaa5e Compare March 10, 2026 21:38

rui-mo closed this by deleting the head repository Mar 30, 2026

rui-mo mentioned this pull request Mar 31, 2026

fix: Change count metric from signed to unsigned (int64_t -> uint64_t) #16989

Open

		int64_t expectedMin = std::numeric_limits<uint64_t>::max(),
		int64_t expectedMax = std::numeric_limits<uint64_t>::min()) {

Conversation

rui-mo commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

majetideepak commented Nov 18, 2025

Uh oh!

rui-mo commented Nov 18, 2025

Uh oh!

tanjialiang left a comment

Choose a reason for hiding this comment

Uh oh!

tanjialiang Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

rui-mo Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

tanjialiang commented Dec 2, 2025

Uh oh!

rui-mo commented Dec 3, 2025

Uh oh!

tanjialiang commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rui-mo commented Dec 10, 2025

Uh oh!

karthikeyann left a comment

Choose a reason for hiding this comment

Uh oh!

rui-mo commented Feb 3, 2026

Uh oh!

rui-mo commented Mar 2, 2026

Uh oh!

Yuhta Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

rui-mo Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

meta-codesync bot commented Mar 5, 2026

Uh oh!

peterenescu commented Mar 6, 2026

Uh oh!

rui-mo commented Mar 6, 2026

Uh oh!

rui-mo commented Mar 6, 2026

Uh oh!

Yuhta commented Mar 6, 2026

Uh oh!

rui-mo commented Mar 10, 2026

Uh oh!

Yuhta commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rui-mo commented Nov 18, 2025 •

edited

Loading

netlify bot commented Nov 18, 2025 •

edited

Loading

tanjialiang commented Dec 4, 2025 •

edited

Loading