-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-11253][SQL] reset all accumulators in physical operators before execute an action #9215
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
() since it has side effect
|
Test build #44126 has finished for PR 9215 at commit
|
|
Test build #44129 has finished for PR 9215 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we run the same plan physical plan multiple times to make sure metrics are good?
|
Can you also double check if our ui looks good after this change? |
|
Test build #44186 has finished for PR 9215 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cloud-fan sorry. I just realized that this method is in the critical path (when we calculate numRows). How about we remove this change and document it clear that those negative initial values will have a small impact on the sum of memory consumption?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to reuse current codebase and didn't create a new SQLMetric(including new MetricValue, MetricParam, etc.). Is it worth to create a new one so that we won't hurt performance for numRows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it is probably not worth that right now. The impact of those -1 just too small (1048576 tasks for 1 MB). I think we can do that later. What do you think?
|
Test build #44237 has finished for PR 9215 at commit
|
|
Test build #44319 has finished for PR 9215 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. Just one last thing. I think we need to call unregister at the end of every test, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
|
Test build #44332 has finished for PR 9215 at commit
|
|
Thanks! Merging to master. |
|
should this merged to branch-1.5? |
With this change, our query execution listener can get the metrics correctly.
The UI still looks good after this change.

