Skip to content

Re-introduce improving the merging of operator stats#24921

Merged
shangm2 merged 1 commit intoprestodb:masterfrom
shangm2:fixBug
Apr 16, 2025
Merged

Re-introduce improving the merging of operator stats#24921
shangm2 merged 1 commit intoprestodb:masterfrom
shangm2:fixBug

Conversation

@shangm2
Copy link
Contributor

@shangm2 shangm2 commented Apr 16, 2025

Description

  1. this pr re-introduce the Optimize how we merge multiple operatorStats #24414 , which cause a sev where written partition was not logged. The bug is a corner case, where while merging only one single non-mergeable operatorInfo, the old code will NOT perform any merge operation (since the add operation will only get invoke when the second operator stats shows up) and give back the operator info itself while Optimize how we merge multiple operatorStats #24414 will actually kick off a merge and gives null result.
  2. This pr reintroduce Optimize how we merge multiple operatorStats #24414 and handles this corner case and also added specific unit tests for this scenario.

Motivation and Context

  1. re-introduce Optimize how we merge multiple operatorStats #24414

Impact

Test Plan

  1. verifier runs log written partition correctly:
Screenshot 2025-04-15 at 17 13 08

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.

Release Notes

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

== RELEASE NOTES ==

General Changes
* Improve how we merge multiple operator stats together.
* Improve metrics creation by refactoring local variables to a dedicated class.

@shangm2 shangm2 requested a review from a team as a code owner April 16, 2025 00:13
@shangm2 shangm2 requested a review from ZacBlanco April 16, 2025 00:13
@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Apr 16, 2025

public List<OperatorStats> getOperatorSummaries()
{
return operatorStatsByKey.values().stream()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@facebook-github-bot
Copy link
Collaborator

@shangm2 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@shangm2 shangm2 merged commit a33f73d into prestodb:master Apr 16, 2025
98 of 99 checks passed
AnuragKDwivedi pushed a commit to AnuragKDwivedi/presto-1 that referenced this pull request Apr 21, 2025
## Description
1. this pr re-introduce the prestodb#24414 , which cause a sev where written
partition was not logged. The bug is a corner case, where while merging
only one single non-mergeable operatorInfo, the old code will NOT
perform any merge operation (since the add operation will only get
invoke when the second operator stats shows up) and give back the
operator info itself while prestodb#24414 will actually kick off a merge and
gives null result.
2. This pr reintroduce prestodb#24414 and handles this corner case and also
added specific unit tests for this scenario.

## Motivation and Context
1. re-introduce prestodb#24414 

## Impact
<!---Describe any public API or user-facing feature change or any
performance impact-->

## Test Plan
1.  verifier runs log written partition correctly:
<img width="1469" alt="Screenshot 2025-04-15 at 17 13 08"
src="https://github.com/user-attachments/assets/f7c84a8f-7381-411a-95d1-15b075870b83"
/>


## Contributor checklist

- [ ] Please make sure your submission complies with our [contributing
guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md),
in particular [code
style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style)
and [commit
standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#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](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines).
- [ ] Adequate tests were added if applicable.
- [ ] CI passed.

## Release Notes
Please follow [release notes
guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines)
and fill in the release notes below.

```
== RELEASE NOTES ==

General Changes
* Improve how we merge multiple operator stats together.
* Improve metrics creation by refactoring local variables to a dedicated class.

```
@ZacBlanco ZacBlanco mentioned this pull request May 29, 2025
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:Meta PR from Meta

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants