Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/query/flamegraph_arrow: Expose diff values, dont set to null #3624

Merged
merged 2 commits into from
Aug 15, 2023

Conversation

metalmatze
Copy link
Member

@metalmatze metalmatze commented Aug 15, 2023

So far, the AppendNull has been used for the first time a row showed up. Even overwriting (adding new diff values) to the row resulted in the value being returned as null, although the underlying value was there.

Old

Notice the blue functions in the first two rows:
image

New

Notice that the functions in the first two rows now have the proper color:
Screenshot from 2023-08-15 13-12-45

@alwaysmeticulous
Copy link

alwaysmeticulous bot commented Aug 15, 2023

✅ Meticulous spotted zero visual differences across 210 screens tested: view results.

Last updated for commit 372a295. This comment will update as new commits are pushed.

@brancz
Copy link
Member

brancz commented Aug 15, 2023

Very nice find! This probably fixes the diffing issues that I've been noticing lately with the new arrow based flamegraph.

So far, the `AppendNull` has been used for the first time a row showed up. Even overwriting (adding new diff values) to the row resulted in the value being returned as null, although the underlying value was there.
@metalmatze metalmatze force-pushed the flamegraph-arrow-diff branch from 0cc3427 to 372a295 Compare August 15, 2023 12:49
@metalmatze metalmatze enabled auto-merge August 15, 2023 13:00
@metalmatze metalmatze added this pull request to the merge queue Aug 15, 2023
Merged via the queue into main with commit 4ca34ee Aug 15, 2023
@metalmatze metalmatze deleted the flamegraph-arrow-diff branch August 17, 2023 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants