You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "HeartSaVioR (via GitHub)" <gi...@apache.org> on 2024/10/17 09:02:49 UTC
[PR] [SPARK-50007][SQL][SS] Provide default values for metrics on observe API when physical node is lost in executed plan [spark]
HeartSaVioR opened a new pull request, #48517:
URL: https://github.com/apache/spark/pull/48517
### What changes were proposed in this pull request?
This PR proposes to provide default values for metrics on observe API, when physical node (CollectMetricsExec) is lost in executed plan. This includes the case where logical node (CollectMetrics) is lost during optimization (and it's mostly the case).
### Why are the changes needed?
When user defines the metrics via observe API, they expect the metrics to be retrieved via Observation (batch query) or update event of StreamingQueryListener.
But when the node (CollectMetrics) is lost in any reason (e.g. subtree is pruned by PruneFilters), Spark does behave like the metrics were not defined, instead of providing default values.
When the query runs successfully, user wouldn't expect the metric being bound to the query to be unavailable, hence they missed to guard the code for this case and encountered some issue. Arguably it's lot better to provide default values - when the node is pruned out from optimizer, it is mostly logically equivalent that there were no input being processed with the node (except the bug in analyzer/optimizer/etc which drop the node incorrectly), hence it's valid to just have default value.
### Does this PR introduce _any_ user-facing change?
Yes, user can consistently query about metrics being defined with observe API. It is available even with aggressive optimization which drop the CollectMetrics(Exec) node.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### 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-50007][SQL][SS] Provide default values for metrics on observe API when physical node is lost in executed plan [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48517:
URL: https://github.com/apache/spark/pull/48517#discussion_r1808666150
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/CollectMetricsExec.scala:
##########
@@ -98,13 +99,30 @@ object CollectMetricsExec extends AdaptiveSparkPlanHelper {
/**
* Recursively collect all collected metrics from a query tree.
*/
- def collect(plan: SparkPlan): Map[String, Row] = {
- val metrics = collectWithSubqueries(plan) {
+ def collect(physicalPlan: SparkPlan, analyzedOpt: Option[LogicalPlan]): Map[String, Row] = {
Review Comment:
when the `analyzedOpt` can be None? tests?
--
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-50007][SQL][SS] Provide default values for metrics on observe API when physical node is lost in executed plan [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #48517:
URL: https://github.com/apache/spark/pull/48517#issuecomment-2428135772
Revert PR: https://github.com/apache/spark/pull/48590
--
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-50007][SQL][SS] Provide default values for metrics on observe API when physical node is lost in executed plan [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #48517:
URL: https://github.com/apache/spark/pull/48517#issuecomment-2421170145
(I got an approval but I'll wait for a full day (7 hours later) to see whether the PR can get another review.)
--
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-50007][SQL][SS] Provide default values for metrics on observe API when physical node is lost in executed plan [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #48517:
URL: https://github.com/apache/spark/pull/48517#issuecomment-2422172150
I'll handle the further review comments as post-review. Thanks! Merging to master.
--
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-50007][SQL][SS] Provide default values for metrics on observe API when physical node is lost in executed plan [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #48517:
URL: https://github.com/apache/spark/pull/48517#issuecomment-2428109032
UPDATE: @hvanhovell and I had an offline talk. I wasn't very clear about the semantic of the API, and he clarified that the intention of the API (semantic) is not to cope with the default values, but "to ensure" the node is executed in any way.
E.g. it is a wrong optimization if any optimization would end up with dropping the node.
I only see the issue for PruneFilters so it's easier to make a point fix, but need to discuss further with other folks as well to aim for better fix.
That said, he also stated that providing default value is worse than not having the metrics; which I agree based on the new understanding of the semantic of the API. Users should be able to know whether Spark fails to calculate the metrics or not. It is at least possible before this fix.
I'll revert the commit and look for a better fix.
--
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-50007][SQL][SS] Provide default values for metrics on observe API when physical node is lost in executed plan [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #48517:
URL: https://github.com/apache/spark/pull/48517#issuecomment-2421063989
Yeah we could even include the "additional" metric entry if it was artificially made with default value, if someone claims that they want to distinguish the case explicitly. I'm not sure there would be the case, so want to defer making this to be more complicated.
--
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-50007][SQL][SS] Provide default values for metrics on observe API when physical node is lost in executed plan [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #48517:
URL: https://github.com/apache/spark/pull/48517#issuecomment-2426695954
@HeartSaVioR how can a `CollectMetricsExec` node get lost? That is somewhat concerning...
--
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-50007][SQL][SS] Provide default values for metrics on observe API when physical node is lost in executed plan [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #48517:
URL: https://github.com/apache/spark/pull/48517#issuecomment-2421062058
cc. @cloud-fan @hvanhovell Could you please take a look? 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-50007][SQL][SS] Provide default values for metrics on observe API when physical node is lost in executed plan [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR closed pull request #48517: [SPARK-50007][SQL][SS] Provide default values for metrics on observe API when physical node is lost in executed plan
URL: https://github.com/apache/spark/pull/48517
--
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-50007][SQL][SS] Provide default values for metrics on observe API when physical node is lost in executed plan [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #48517:
URL: https://github.com/apache/spark/pull/48517#discussion_r1809639753
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/CollectMetricsExec.scala:
##########
@@ -98,13 +99,30 @@ object CollectMetricsExec extends AdaptiveSparkPlanHelper {
/**
* Recursively collect all collected metrics from a query tree.
*/
- def collect(plan: SparkPlan): Map[String, Row] = {
- val metrics = collectWithSubqueries(plan) {
+ def collect(physicalPlan: SparkPlan, analyzedOpt: Option[LogicalPlan]): Map[String, Row] = {
Review Comment:
Please take a look at the pattern for InMemoryTableScanExec: we may not have logical link for the physical plan.
--
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