fix(native): Caps the count metric at int64_t's max value by rui-mo · Pull Request #27295 · prestodb/presto

rui-mo · 2026-03-09T16:09:27Z

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

Impact

Ensures no overflow when converting the uint64 count metric to int64.

Test Plan

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.
If adding new dependencies, verified they have an OpenSSF Scorecard score of 5.0 or higher (or obtained explicit TSC approval for lower scores).

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... 
* ... 

Hive Connector Changes
* ... 
* ...

If release note is NOT required, use:

== NO RELEASE NOTE ==

sourcery-ai · 2026-03-09T16:09:35Z

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Changes the native execution runtime metrics structure so that the count field uses an unsigned 64-bit integer type instead of a signed 64-bit integer, aligning Presto’s count metric representation with Velox and preventing overflow/negative counts while leaving other metric fields unchanged.

Updated class diagram for RuntimeMetric count type change

classDiagram
class RuntimeMetric_before {
  String name
  RuntimeUnit unit
  int64_t sum
  int64_t count
  int64_t max
  int64_t min
}

class RuntimeMetric {
  String name
  RuntimeUnit unit
  int64_t sum
  uint64_t count
  int64_t max
  int64_t min
}

RuntimeMetric_before <.. RuntimeMetric : count field changed to uint64_t

File-Level Changes

Change	Details	Files
Use an unsigned 64-bit integer type for the runtime metric count field while keeping other metric fields as signed 64-bit integers.	Updated the RuntimeMetric struct to change the count field type from int64_t to uint64_t. Left sum, max, and min fields as int64_t to continue representing value metrics with signed integers. Relies on upstream Velox refactor so Presto native metrics no longer risk overflow or negative values when mapping count metrics.	`presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.h`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey - I've left some high level feedback:

Since RuntimeMetric::count is part of a protocol header, double-check all serialization/deserialization and wire formats (e.g., JSON, Thrift, Velox interop) to ensure the signed→unsigned change is reflected consistently and does not break backward compatibility with existing consumers.
Audit all arithmetic and comparisons involving RuntimeMetric::count to ensure there are no remaining implicit conversions between signed and unsigned types that could introduce subtle bugs or compiler warnings.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Since `RuntimeMetric::count` is part of a protocol header, double-check all serialization/deserialization and wire formats (e.g., JSON, Thrift, Velox interop) to ensure the signed→unsigned change is reflected consistently and does not break backward compatibility with existing consumers.
- Audit all arithmetic and comparisons involving `RuntimeMetric::count` to ensure there are no remaining implicit conversions between signed and unsigned types that could introduce subtle bugs or compiler warnings.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

pramodsatya

Thanks @rui-mo.

pramodsatya · 2026-03-09T17:32:16Z

presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.h

  RuntimeUnit unit = {};
  int64_t sum = {};
-  int64_t count = {};
+  uint64_t count = {};


This is generated via presto protocol with these steps from the java class definition here.

Once velox advances to include int64_t -> uint64_t change, could you add a static_cast in Presto where necessary, instead of changing here?

Thanks for letting me know! Based on your suggestion, we’ll probably need this PR: #27276. Could you please advise on the preferred order for merging? Would it be better to merge the Velox refactor first, or should we merge this fix into Presto first to avoid potentially breaking anything?

Yes the linked PR would work, I think it is better to advance velox submodule and make the Presto metric fix in the same commit to avoid breaking anything.

Thanks for confirming!

Hi @pramodsatya, after discussing with the Velox community, it turns out the Velox CI jobs won’t pass without this Presto layer to ensure no overflow occurs. Could you help review and land this patch first? I’ll switch to using saturateCast in a follow-up for simplicity once the Velox submodule is advanced. Thanks for your help!

pramodsatya

Thanks @rui-mo.

pramodsatya · 2026-03-16T21:15:10Z

presto-native-execution/presto_cpp/main/PrestoTask.cpp

    const RuntimeMetric& metric) {
+  // 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.
+  auto count = static_cast<int64_t>(std::min(


nit: const auto

Thanks for the prompt feedback. I've made the update.

aditi-pandit

Thanks @rui-mo for this code.

aditi-pandit · 2026-03-16T21:28:46Z

presto-native-execution/presto_cpp/main/PrestoTask.cpp

+  // uses uint64_t. To avoid overflow, we cap the count at int64_t's max value.
+  const auto count = static_cast<int64_t>(std::min(
+      metric.count,
+      static_cast<uint64_t>(std::numeric_limits<int64_t>::max())));


Can you abstract this as a static constant as its used per toRuntimeMetric call ?

Thanks for helping review. I made the update.

rui-mo · 2026-03-18T10:31:32Z

Hi @pramodsatya @aditi-pandit, thanks for the review! If there are no further comments, can we go ahead and land this change?

aditi-pandit

Thanks @rui-mo. Just one minor comment.

presto-native-execution/presto_cpp/main/PrestoTask.cpp

aditi-pandit

Thanks @rui-mo

rui-mo · 2026-03-30T20:35:39Z

Hi @aditi-pandit @pramodsatya, could you please help land this change if no further comments? Thanks!

NivinCS

Thanks for the change. LGTM.

rui-mo · 2026-03-31T20:35:53Z

Thanks everyone for the reviews and for helping get this PR merged. I’ll follow up with cleanup once the Velox version is updated.

…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 == ```

rui-mo requested review from a team as code owners March 9, 2026 16:09

prestodb-ci added the from:IBM PR from IBM label Mar 9, 2026

prestodb-ci requested review from a team, NivinCS and pramodsatya and removed request for a team March 9, 2026 16:09

sourcery-ai bot reviewed Mar 9, 2026

View reviewed changes

pramodsatya requested changes Mar 9, 2026

View reviewed changes

This was referenced Mar 10, 2026

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

Closed

fix: Change count metric from signed to unsigned (int64_t -> uint64_t) facebookincubator/velox#15536

Closed

rui-mo force-pushed the wip_count branch from b0615eb to 8b36a76 Compare March 16, 2026 20:25

rui-mo changed the title ~~fix(native): Change count metric from signed to unsigned (int64_t -> uint64_t)~~ fix(native): Caps the count metric at int64_t's max value Mar 16, 2026

pramodsatya previously approved these changes Mar 16, 2026

View reviewed changes

rui-mo dismissed pramodsatya’s stale review via b839aff March 16, 2026 21:18

rui-mo force-pushed the wip_count branch from 8b36a76 to b839aff Compare March 16, 2026 21:18

aditi-pandit reviewed Mar 16, 2026

View reviewed changes

rui-mo force-pushed the wip_count branch from b839aff to 74317cf Compare March 16, 2026 21:32

fix: Caps the count metric at int64_t's max value

650a330

rui-mo force-pushed the wip_count branch from 74317cf to 650a330 Compare March 17, 2026 23:35

aditi-pandit reviewed Mar 19, 2026

View reviewed changes

presto-native-execution/presto_cpp/main/PrestoTask.cpp Show resolved Hide resolved

aditi-pandit approved these changes Mar 19, 2026

View reviewed changes

NivinCS approved these changes Mar 31, 2026

View reviewed changes

aditi-pandit merged commit 7f97f8d into prestodb:master Mar 31, 2026
144 of 147 checks passed

rui-mo mentioned this pull request Mar 31, 2026

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

Open

prestodb-ci mentioned this pull request Apr 1, 2026

docs: Add release notes for 0.297 #27484

Open

15 tasks

Conversation

rui-mo commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

sourcery-ai bot commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide

Updated class diagram for RuntimeMetric count type change

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

pramodsatya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rui-mo Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pramodsatya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rui-mo commented Mar 18, 2026

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

rui-mo commented Mar 30, 2026

Uh oh!

NivinCS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rui-mo commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rui-mo commented Mar 9, 2026 •

edited

Loading

sourcery-ai bot commented Mar 9, 2026 •

edited

Loading

rui-mo Mar 16, 2026 •

edited

Loading