Skip to content

[SPARK-50315][SQL] Support custom metrics for V1Fallback writes - #48867

Closed
olaky wants to merge 5 commits into
apache:masterfrom
olaky:sc-50315-metrics-for-v1-fallback-writers
Closed

[SPARK-50315][SQL] Support custom metrics for V1Fallback writes#48867
olaky wants to merge 5 commits into
apache:masterfrom
olaky:sc-50315-metrics-for-v1-fallback-writers

Conversation

@olaky

@olakyolaky commented Nov 16, 2024

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Support for custom metrics for V1Fallback writers (AppendDataExecV1, OverwriteByExpressionExecV1)

Why are the changes needed?

  • Add the custom metrics of the V1Write as metrics to the V1FallbackWriters implementations
  • Publish the metrics from reportDriverMetrics

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added a new test on top of mocked implementations

Was this patch authored or co-authored using generative AI tooling?

No

@olaky

olaky commented Nov 16, 2024

Copy link
Copy Markdown
ContributorAuthor

cc @cloud-fan@manuzhang since you recently worked on similar changes

cloud-fan
cloud-fan reviewed Nov 18, 2024
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V1FallbackWriters.scala Outdated
val executionId = sparkContext.getLocalProperty(SQLExecution.EXECUTION_ID_KEY)
SQLMetrics.postDriverMetricUpdates(sparkContext, executionId, metrics.values.toSeq)

metrics.get("numOutputRows").map(metric => InternalRow(metric.value, metric.value)).toSeq

@cloud-fancloud-fanNov 18, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We always return Nil before this PR, we should keep this behavior.

@olakyolakyNov 18, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

cloud-fan
cloud-fan reviewed Nov 18, 2024
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/connector/V1WriteFallbackSuite.scala Outdated
}

test("SPARK-50315: metrics for V1 fallback writers") {
val session = SparkSession.builder()

@cloud-fancloud-fanNov 18, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all other tests in this suite do SparkSession.clearActiveSession();SparkSession.clearDefaultSession() at the beginning, shall we follow it here?

@olakyolakyNov 18, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not add it because it worked without as I wanted, but let's assume the code is there in the other tests for a reason

cloud-fan
cloud-fan reviewed Nov 18, 2024
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/connector/V1WriteFallbackSuite.scala Outdated
}
assert(appendPlan.metrics("numOutputRows").value === 1)

session.catalog.cacheTable("test")

@cloud-fancloud-fanNov 18, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to cache it?

@olakyolakyNov 18, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oversight, no need to do that

@olaky

olaky commented Nov 18, 2024

Copy link
Copy Markdown
ContributorAuthor

@cloud-fan all comments addressed

@HyukjinKwonHyukjinKwon changed the title [SPARK-50315] Support custom metrics for V1Fallback writes[SPARK-50315][SQL] Support custom metrics for V1Fallback writesNov 19, 2024
@cloud-fan

cloud-fan commented Nov 19, 2024

Copy link
Copy Markdown
Contributor

thanks, merging to master!

@pan3793pan3793 mentioned this pull request Feb 27, 2026
Closed
dongjoon-hyun pushed a commit that referenced this pull request Feb 27, 2026
@pan3793@dongjoon-hyun
[SPARK-55746][SQL][TESTS] Fix unable to load custom metric object Sup…
a0a092f
…portedV1WriteMetric
### What changes were proposed in this pull request?
Bug was introduced by SPARK-50315 (#48867), won't fail the test, just causes lots of warning logs
```
$ build/sbt "sql/testOnly *V1WriteFallbackSuite"
...
18:06:25.108 WARN org.apache.spark.sql.execution.ui.SQLAppStatusListener: Unable to load custom metric object for class `org.apache.spark.sql.connector.SupportedV1WriteMetric`. Please make sure that the custom metric class is in the classpath and it has 0-arg constructor.
org.apache.spark.SparkException: org.apache.spark.sql.connector.SupportedV1WriteMetric did not have a zero-argument constructor or a single-argument constructor that accepts SparkConf. Note: if the class is defined inside of another Scala class, then its constructors may accept an implicit parameter that references the enclosing class; in this case, you must define the class as a top-level class in order to prevent this extra parameter from breaking Spark's ability to find a valid constructor.
at org.apache.spark.util.Utils$.$anonfun$loadExtensions$1(Utils.scala:2871)
at scala.collection.immutable.List.flatMap(List.scala:283)
at scala.collection.immutable.List.flatMap(List.scala:79)
at org.apache.spark.util.Utils$.loadExtensions(Utils.scala:2853)
at org.apache.spark.sql.execution.ui.SQLAppStatusListener.$anonfun$aggregateMetrics$3(SQLAppStatusListener.scala:220)
at scala.Option.map(Option.scala:242)
at org.apache.spark.sql.execution.ui.SQLAppStatusListener.$anonfun$aggregateMetrics$2(SQLAppStatusListener.scala:214)
... (repeat many times)
```
### Why are the changes needed?
Fix UT.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Verified locally, no warnings printed after fixing.
### Was this patch authored or co-authored using generative AI tooling?
No.
Closes#54544 from pan3793/SPARK-55746.
Authored-by: Cheng Pan <chengpan@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
dongjoon-hyun pushed a commit that referenced this pull request Feb 27, 2026
@pan3793@dongjoon-hyun
[SPARK-55746][SQL][TESTS] Fix unable to load custom metric object Sup…
a747590
…portedV1WriteMetric
### What changes were proposed in this pull request?
Bug was introduced by SPARK-50315 (#48867), won't fail the test, just causes lots of warning logs
```
$ build/sbt "sql/testOnly *V1WriteFallbackSuite"
...
18:06:25.108 WARN org.apache.spark.sql.execution.ui.SQLAppStatusListener: Unable to load custom metric object for class `org.apache.spark.sql.connector.SupportedV1WriteMetric`. Please make sure that the custom metric class is in the classpath and it has 0-arg constructor.
org.apache.spark.SparkException: org.apache.spark.sql.connector.SupportedV1WriteMetric did not have a zero-argument constructor or a single-argument constructor that accepts SparkConf. Note: if the class is defined inside of another Scala class, then its constructors may accept an implicit parameter that references the enclosing class; in this case, you must define the class as a top-level class in order to prevent this extra parameter from breaking Spark's ability to find a valid constructor.
at org.apache.spark.util.Utils$.$anonfun$loadExtensions$1(Utils.scala:2871)
at scala.collection.immutable.List.flatMap(List.scala:283)
at scala.collection.immutable.List.flatMap(List.scala:79)
at org.apache.spark.util.Utils$.loadExtensions(Utils.scala:2853)
at org.apache.spark.sql.execution.ui.SQLAppStatusListener.$anonfun$aggregateMetrics$3(SQLAppStatusListener.scala:220)
at scala.Option.map(Option.scala:242)
at org.apache.spark.sql.execution.ui.SQLAppStatusListener.$anonfun$aggregateMetrics$2(SQLAppStatusListener.scala:214)
... (repeat many times)
```
### Why are the changes needed?
Fix UT.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Verified locally, no warnings printed after fixing.
### Was this patch authored or co-authored using generative AI tooling?
No.
Closes#54544 from pan3793/SPARK-55746.
Authored-by: Cheng Pan <chengpan@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit a0a092f)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
dongjoon-hyun pushed a commit that referenced this pull request Feb 27, 2026
@pan3793@dongjoon-hyun
[SPARK-55746][SQL][TESTS] Fix unable to load custom metric object Sup…
d69d94d
…portedV1WriteMetric
### What changes were proposed in this pull request?
Bug was introduced by SPARK-50315 (#48867), won't fail the test, just causes lots of warning logs
```
$ build/sbt "sql/testOnly *V1WriteFallbackSuite"
...
18:06:25.108 WARN org.apache.spark.sql.execution.ui.SQLAppStatusListener: Unable to load custom metric object for class `org.apache.spark.sql.connector.SupportedV1WriteMetric`. Please make sure that the custom metric class is in the classpath and it has 0-arg constructor.
org.apache.spark.SparkException: org.apache.spark.sql.connector.SupportedV1WriteMetric did not have a zero-argument constructor or a single-argument constructor that accepts SparkConf. Note: if the class is defined inside of another Scala class, then its constructors may accept an implicit parameter that references the enclosing class; in this case, you must define the class as a top-level class in order to prevent this extra parameter from breaking Spark's ability to find a valid constructor.
at org.apache.spark.util.Utils$.$anonfun$loadExtensions$1(Utils.scala:2871)
at scala.collection.immutable.List.flatMap(List.scala:283)
at scala.collection.immutable.List.flatMap(List.scala:79)
at org.apache.spark.util.Utils$.loadExtensions(Utils.scala:2853)
at org.apache.spark.sql.execution.ui.SQLAppStatusListener.$anonfun$aggregateMetrics$3(SQLAppStatusListener.scala:220)
at scala.Option.map(Option.scala:242)
at org.apache.spark.sql.execution.ui.SQLAppStatusListener.$anonfun$aggregateMetrics$2(SQLAppStatusListener.scala:214)
... (repeat many times)
```
### Why are the changes needed?
Fix UT.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Verified locally, no warnings printed after fixing.
### Was this patch authored or co-authored using generative AI tooling?
No.
Closes#54544 from pan3793/SPARK-55746.
Authored-by: Cheng Pan <chengpan@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit a0a092f)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fancloud-fancloud-fan approved these changes
@HyukjinKwonHyukjinKwonHyukjinKwon approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@olaky@cloud-fan@HyukjinKwon