fix: Live plan not showing and output data size is undefined #26033
+36
−39
Conversation
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
prestodb-ci
requested review from
a team,
Dilli-Babu-Godari and
namya28
and removed request for
a team
Reviewer's GuideThis PR refactors UI components to use the new byte-based data size properties and proper formatting, ensures the LivePlan visualization always updates, and addresses React warning in the query header tabs. Class diagram for updated data size properties in Operator and Stage statisticsclassDiagram
class Operator {
+inputDataSizeInBytes
+outputDataSizeInBytes
+inputPositions
+outputPositions
+Other attributes
}
class StageStatistics {
+bufferedDataSizeInBytes
+userMemoryReservationInBytes
+rawInputDataSizeInBytes
+outputDataSizeInBytes
+Other attributes
}
Operator --> StageStatistics: used in statistics display
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider formatting the raw byte count (outputDataSizeInBytes) into a human-readable string (KB/MB/GB) rather than displaying the raw number to match the rest of the UI.
- Instead of unconditionally calling updateD3Graph in componentDidUpdate, add a guard to only run it when the underlying plan data actually changes to avoid unnecessary graph re-renders.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider formatting the raw byte count (outputDataSizeInBytes) into a human-readable string (KB/MB/GB) rather than displaying the raw number to match the rest of the UI.
- Instead of unconditionally calling updateD3Graph in componentDidUpdate, add a guard to only run it when the underlying plan data actually changes to avoid unnecessary graph re-renders.
## Individual Comments
### Comment 1
<location> `presto-ui/src/components/LivePlan.jsx:253` </location>
<code_context>
style: "stroke-width: 4px",
arrowheadClass: "plan-arrowhead",
- label: sourceStats.outputDataSize + " / " + formatRows(sourceStats.outputPositions),
+ label: sourceStats.outputDataSizeInBytes + " bytes / " + formatRows(sourceStats.outputPositions),
labelStyle: "color: #fff; font-weight: bold; font-size: 24px;",
labelType: "html",
</code_context>
<issue_to_address>
Consider using a human-readable format for byte size display.
Formatting the byte size as KB, MB, or GB will make the label easier for users to interpret.
Suggested implementation:
```javascript
label: formatBytes(sourceStats.outputDataSizeInBytes) + " / " + formatRows(sourceStats.outputPositions),
```
```javascript
}
// Utility function to format bytes as human-readable string
function formatBytes(bytes) {
if (bytes === 0) return '0 Bytes';
const k = 1024;
const sizes = ['Bytes', 'KB', 'MB', 'GB', 'TB', 'PB'];
const i = Math.floor(Math.log(bytes) / Math.log(k));
return parseFloat((bytes / Math.pow(k, i)).toFixed(2)) + ' ' + sizes[i];
}
```
</issue_to_address>
### Comment 2
<location> `presto-ui/src/components/QueryHeader.jsx:118` </location>
<code_context>
<div className="col-6 d-flex justify-content-end">
<nav className="nav nav-tabs">
{tabs.map((page, _) => (
- <>
+ <React.Fragment key={page.label}>
<a className={clsx('nav-link', 'navbar-btn', this.isActive(page.path) && 'active')} href={page.path + '?' + queryId} >{page.label}</a>
</code_context>
<issue_to_address>
Using page.label as a key may cause issues if labels are not unique.
Using non-unique keys can lead to React rendering problems. Please use a unique value like page.path for the key.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
<React.Fragment key={page.label}>
=======
<React.Fragment key={page.path}>
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
unidevel
force-pushed
the
uifix
branch
3 times, most recently
from
ad58d76 to
fc6432a
Compare
unidevel
changed the title
|
Another reason to work on #25744, it would have caught this problem. |
rschlussel
reviewed
Sep 15, 2025
| style: "stroke-width: 4px", | ||
| arrowheadClass: "plan-arrowhead", | ||
| label: sourceStats.outputDataSize + " / " + formatRows(sourceStats.outputPositions), | ||
| label: sourceStats.outputDataSizeInBytes + " / " + formatRows(sourceStats.outputPositions), |
There was a problem hiding this comment.
will this be less user friendly/readable then previously (e.g. would it previously show 10GB, and now will show 10000000000 bytes)? Can we convert it back to something that will be readable? Same comment for the rest of the usages
There was a problem hiding this comment.
utils.formatDataSizeBytes() could be used here to improve the readability
|
Also,
|
|
Just found there are many other issues about the |
unidevel
force-pushed
the
uifix
branch
2 times, most recently
from
d58e73e to
32ee924
Compare
|
@rschlussel here are the screenshots Live plan
Stage Performance
Pipeline details
|
|
@unidevel imported this issue as lakehouse/presto #26033 |
unidevel
changed the title
unidevel
changed the title
yhwang
reviewed
Oct 6, 2025
| style: "stroke-width: 4px", | ||
| arrowheadClass: "plan-arrowhead", | ||
| label: sourceStats.outputDataSize + " / " + formatRows(sourceStats.outputPositions), | ||
| label: sourceStats.outputDataSizeInBytes + " / " + formatRows(sourceStats.outputPositions), |
There was a problem hiding this comment.
do we need to format this part too? just curious
There was a problem hiding this comment.
this should match to the LivePlan in LivePlan.jsx
There was a problem hiding this comment.
Thanks @yhwang , fixed
|
Hi @rschlussel
I think this would be a working item for the UI. We should trace this in another issue and new PR to address it. Are you okay with the this? |
|
The lack of tests are certainly something we should address, but it's a pre-existing issue. This makes the UI usable again, so we should merge this. |
|
@unidevel we should also make sure this lands in |
rschlussel
approved these changes
Oct 9, 2025
6 tasks
|
@tdcmeehan I added the PR for 0.295 release and did a quick test with this new PR, it looks fine in my local env. => #26264 |
tdcmeehan
approved these changes
Oct 9, 2025
unidevel
added a commit
that referenced
this pull request
Oct 9, 2025
## Description 1. Fix live plan not showing <img width="2448" height="1730" alt="image" src="https://github.com/user-attachments/assets/25698228-0d62-4326-b0c7-41c1ed3af579" /> 2. Fix warnings in Query Header 3. Fix output data size is `undefined` - due to [this change](7458b52) , `outputDataSize` is changed to `outputDataSizeInBytes` ## Motivation and Context PR for master branch: #26033 ## Impact UI ## Test Plan Manual test ## 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 ``` == NO RELEASE NOTE == ``` --------- Co-authored-by: Timothy Meehan <tim@timdmeehan.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Description
Fix warnings in Query Header
Fix output data size is
undefined- due to this change ,outputDataSizeis changed tooutputDataSizeInBytesMotivation and Context
Impact
UI
Test Plan
Manual test
Contributor checklist
Release Notes