You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "jiwen624 (via GitHub)" <gi...@apache.org> on 2025/02/24 03:00:17 UTC
[PR] [WIP][SPARK-51299][SQL][Web UI] Filter out invalid metric values with initValue [spark]
jiwen624 opened a new pull request, #50055:
URL: https://github.com/apache/spark/pull/50055
### What changes were proposed in this pull request?
This PR proposes to use `initValue` of a metric in `org.apache.spark.util.MetricUtils.stringValue` instead of a hardcoded init value to filter out invalid metric values.
### Why are the changes needed?
In method `org.apache.spark.util.MetricUtils.stringValue`, it uses a hardcoded value 0 to filter out invalid metric values for SIZE_METRIC, TIMING_METRIC and NS_TIMING_METRIC:
`val validValues = values.filter(_ >= 0)
`
However, we offer methods to create these types of metrics with initValue other than -1 (introduced in this PR https://github.com/apache/spark/pull/41555) :
`def createSizeMetric(sc: SparkContext, name: String, initValue: Long = -1): SQLMetric = {
`
which means there is a chance that the metrics are created with a initValue != -1 and in this case the filter above will generate incorrect results.
### Does this PR introduce any user-facing change?
No
### How was this patch tested?
Existing UTs
Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51299][SQL][UI] MetricUtils.stringValue should filter metric values with initValue rather than a hardcoded value [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #50055:
URL: https://github.com/apache/spark/pull/50055#issuecomment-2678515900
I'm not very convinced by the "Why" section. What's the end-to-end problem you hit?
BTW https://github.com/apache/spark/pull/47721 has some more context about the SQLMetric initial value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51299][SQL][UI] MetricUtils.stringValue should filter metric values with initValue rather than a hardcoded value [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #50055:
URL: https://github.com/apache/spark/pull/50055#issuecomment-2688019216
@jiwen624 I think you are right, do you have a concrete metric that has the problem? For metrics that we only need a sum, `0` is OK as the initial value and we don't filter it out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51299][SQL][UI] MetricUtils.stringValue should filter metric values with initValue rather than a hardcoded value [spark]
Posted by "jiwen624 (via GitHub)" <gi...@apache.org>.
jiwen624 commented on PR #50055:
URL: https://github.com/apache/spark/pull/50055#issuecomment-2677443309
@dongjoon-hyun @cloud-fan could you let me know your thoughts about this fix? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51299][SQL][UI] MetricUtils.stringValue should filter metric values with initValue rather than a hardcoded value [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #50055: [SPARK-51299][SQL][UI] MetricUtils.stringValue should filter metric values with initValue rather than a hardcoded value
URL: https://github.com/apache/spark/pull/50055
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51299][SQL][UI] MetricUtils.stringValue should filter metric values with initValue rather than a hardcoded value [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50055:
URL: https://github.com/apache/spark/pull/50055#issuecomment-2953298123
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51299][SQL][UI] MetricUtils.stringValue should filter metric values with initValue rather than a hardcoded value [spark]
Posted by "jiwen624 (via GitHub)" <gi...@apache.org>.
jiwen624 commented on PR #50055:
URL: https://github.com/apache/spark/pull/50055#issuecomment-2683915590
@cloud-fan Thanks a lot for the response and the context in https://github.com/apache/spark/pull/47721. It's very helpful.
From what I see in previous PRs and comments in the code, it seems that we treat `initValue` in SQLMetric as a invalid value which should be filtered out. For example, in this recent PR https://github.com/apache/spark/pull/44222, it mentions:
> initValue is the starting value for a SQLMetric. If a metric has value equal to its initValue, then it can/should be filtered out before aggregating with SQLMetrics.stringValue().
and here the comment about the idea of having -1 indicates that it's originally introduced to "distinguish whether a SQLMetric is initialized or not" : https://github.com/apache/spark/pull/26899#discussion_r358044877
and the comment in the code [here](https://github.com/apache/spark/blob/a452e1bc2189d4dda50df5c36a49e4d23e6db758/sql/core/src/main/scala/org/apache/spark/sql/execution/metric/SQLMetrics.scala#L68-L69):
> Metrics with value equal to initValue should be filtered out, since they are either invalid or safe to filter without changing...
However, in [`MetricUtils.stringValue`](https://github.com/apache/spark/blob/a452e1bc2189d4dda50df5c36a49e4d23e6db758/core/src/main/scala/org/apache/spark/util/MetricUtils.scala#L88), it keeps `0` as a valid value in the filter logic, even it's possible that the `initValue` for these metrics can be `0`. This seems to contradict the semantics of initValue and makes the invalid/uninitialized values pollute the `min, med...` calculation - for example, the `med` may point to an unintialized/initValue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org