[SPARK-41442][SQL] Only update SQLMetric value if merging with valid metric #38969
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
Member
Author
Member
Author
|
Thank you @sunchao @dongjoon-hyun ! Pending on CI (not sure if any existing tests depend on current behavior). |
Member
|
Could you re-trigger the failure CI test only? |
Member
Author
|
I can reproduce the failed tests. Trying to fix them. |
| if (rowCount == 0) { | ||
| // For empty relation, the query stage doesn't serialize any bytes. | ||
| // The SQLMetric keeps initial value. | ||
| assert(stats.sizeInBytes == -1) |
dongjoon-hyun
approved these changes
Dec 8, 2022
HeartSaVioR
approved these changes
Dec 8, 2022
Contributor
HeartSaVioR
left a comment
HeartSaVioR
left a comment
There was a problem hiding this comment.
+1 It totally makes sense.
Member
|
All tests passed. Merged to master. Thank you, @viirya and all! |
Member
Author
|
Thank you! |
cloud-fan
reviewed
Dec 12, 2022
| // The exchange doesn't serialize any bytes. | ||
| // The SQLMetric keeps initial value. | ||
| testMetricsInSparkPlanOperator(exchanges(1), | ||
| Map("dataSize" -> -1, "shuffleRecordsWritten" -> 0)) |
Contributor
There was a problem hiding this comment.
what do we show in the SQL UI? printing -1 as the size is a bit weird.
Member
Author
There was a problem hiding this comment.
I think -1 will be filtered out.
beliefer
pushed a commit
to beliefer/spark
that referenced
this pull request
Dec 18, 2022
…metric ### What changes were proposed in this pull request? This patch updates how `SQLMetric` merges two invalid instances where the value is both -1. ### Why are the changes needed? We use -1 as initial value of `SQLMetric`, and change it to 0 while merging with other `SQLMetric` instances. A `SQLMetric` will be treated as invalid and filtered out later. While we are developing with Spark, it is trouble behavior that two invalid `SQLMetric` instances merge to a valid `SQLMetric` because merging will set the value to 0. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing tests. Closes apache#38969 from viirya/minor_sql_metrics. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
HyukjinKwon
pushed a commit
that referenced
this pull request
Dec 31, 2022
…it's invalid ### What changes were proposed in this pull request? This is a followup of #38969 to fix a regression. SQL UI is not the only way for end users to see the SQL metrics. They can also access the accumulator values in the physical plan programmatically via query execution listener. We should be consistent with the SQL UI and not expose the -1 value. ### Why are the changes needed? make SQL UI and the accumulator value consistent ### Does this PR introduce _any_ user-facing change? Yes, as end users can access accumulator values directly. ### How was this patch tested? existing tests Closes #39311 from cloud-fan/metric. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
sunchao
pushed a commit
to sunchao/spark
that referenced
this pull request
Jun 2, 2023
…metric (apache#1643) ### What changes were proposed in this pull request? This patch updates how `SQLMetric` merges two invalid instances where the value is both -1. ### Why are the changes needed? We use -1 as initial value of `SQLMetric`, and change it to 0 while merging with other `SQLMetric` instances. A `SQLMetric` will be treated as invalid and filtered out later. While we are developing with Spark, it is trouble behavior that two invalid `SQLMetric` instances merge to a valid `SQLMetric` because merging will set the value to 0. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing tests. Closes apache#38969 from viirya/minor_sql_metrics. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
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.
What changes were proposed in this pull request?
This patch updates how
SQLMetricmerges two invalid instances where the value is both -1.Why are the changes needed?
We use -1 as initial value of
SQLMetric, and change it to 0 while merging with otherSQLMetricinstances. ASQLMetricwill be treated as invalid and filtered out later.While we are developing with Spark, it is trouble behavior that two invalid
SQLMetricinstances merge to a validSQLMetricbecause merging will set the value to 0.Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing tests.