-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-34338][SQL] Report metrics from Datasource v2 scan #31451
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
Changes from 1 commit
4d941c7
f72834c
4215ec0
d3bf283
aedb965
918c90d
e8576ec
d5d8678
cf05fb7
6dced9c
a50bf40
7d50027
b5be5ba
0f38782
50ed317
06eb9c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.spark.sql.connector.read; | ||
|
|
||
| import org.apache.spark.annotation.Evolving; | ||
|
|
||
| /** | ||
| * A custom metric for {@link Scan}. | ||
| * | ||
| * @since 3.2.0 | ||
| */ | ||
| @Evolving | ||
| public interface CustomMetric { | ||
| /** | ||
| * Returns the name of custom metric. | ||
| */ | ||
| String getName(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find that I think it would be better to name these
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. Let me update it in next commit. |
||
|
|
||
| /** | ||
| * Returns the description of custom metric. | ||
| */ | ||
| String getDescription(); | ||
|
|
||
| /** | ||
| * Returns the value of custom metric. | ||
| */ | ||
| Long getValue(); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ import org.apache.spark.rdd.RDD | |
| import org.apache.spark.sql.catalyst.InternalRow | ||
| import org.apache.spark.sql.catalyst.expressions._ | ||
| import org.apache.spark.sql.catalyst.plans.QueryPlan | ||
| import org.apache.spark.sql.connector.read.{InputPartition, PartitionReaderFactory, Scan} | ||
| import org.apache.spark.sql.connector.read.{InputPartition, PartitionReader, PartitionReaderFactory, Scan} | ||
|
|
||
| /** | ||
| * Physical plan node for scanning a batch of data from a data source v2. | ||
|
|
@@ -44,8 +44,19 @@ case class BatchScanExec( | |
|
|
||
| override lazy val readerFactory: PartitionReaderFactory = batch.createReaderFactory() | ||
|
|
||
| /** | ||
| * The callback function which is called when the output iterator of input RDD is consumed | ||
| * completely. | ||
| */ | ||
| private def onOutputCompletion(reader: PartitionReader[_]) = { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have the plan to do other things on completion? Otherwise, I'd like to suggest naming
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good to me. |
||
| reader.getCustomMetrics.foreach { metric => | ||
| longMetric(metric.getName) += metric.getValue | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if the metric doesn't exist in the SQLMetrics in case of users implemented mistakenly? Would it be better to throw an explicit error?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. I will do it. |
||
| } | ||
| } | ||
|
|
||
| override lazy val inputRDD: RDD[InternalRow] = { | ||
| new DataSourceRDD(sparkContext, partitions, readerFactory, supportsColumnar) | ||
| new DataSourceRDD(sparkContext, partitions, readerFactory, supportsColumnar, | ||
| onOutputCompletion) | ||
| } | ||
|
|
||
| override def doCanonicalize(): BatchScanExec = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ package org.apache.spark.sql.execution.datasources.v2 | |
| import org.apache.spark.rdd.RDD | ||
| import org.apache.spark.sql.catalyst.InternalRow | ||
| import org.apache.spark.sql.catalyst.expressions._ | ||
| import org.apache.spark.sql.connector.read.{InputPartition, Scan} | ||
| import org.apache.spark.sql.connector.read.{InputPartition, PartitionReader, Scan} | ||
| import org.apache.spark.sql.connector.read.streaming.{ContinuousPartitionReaderFactory, ContinuousStream, Offset} | ||
| import org.apache.spark.sql.execution.streaming.continuous._ | ||
|
|
||
|
|
@@ -47,6 +47,16 @@ case class ContinuousScanExec( | |
| stream.createContinuousReaderFactory() | ||
| } | ||
|
|
||
| /** | ||
| * The callback function which is called when the output iterator of input RDD is consumed | ||
| * completely. | ||
| */ | ||
| private def onOutputCompletion(reader: PartitionReader[_]) = { | ||
| reader.getCustomMetrics.foreach { metric => | ||
| longMetric(metric.getName) += metric.getValue | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also have a way to set task input metrics? Maybe a reserved metric name like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 will add in later implementation PRs. |
||
| } | ||
|
|
||
| override lazy val inputRDD: RDD[InternalRow] = { | ||
| EpochCoordinatorRef.get( | ||
| sparkContext.getLocalProperty(ContinuousExecution.EPOCH_COORDINATOR_ID_KEY), | ||
|
|
@@ -58,6 +68,7 @@ case class ContinuousScanExec( | |
| sqlContext.conf.continuousStreamingExecutorPollIntervalMs, | ||
| partitions, | ||
| schema, | ||
| readerFactory.asInstanceOf[ContinuousPartitionReaderFactory]) | ||
| readerFactory.asInstanceOf[ContinuousPartitionReaderFactory], | ||
| onOutputCompletion) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,8 +32,13 @@ import org.apache.spark.util.Utils | |
|
|
||
| trait DataSourceV2ScanExecBase extends LeafExecNode { | ||
|
|
||
| override lazy val metrics = Map( | ||
| "numOutputRows" -> SQLMetrics.createMetric(sparkContext, "number of output rows")) | ||
| override lazy val metrics = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any plans to similarly support metrics in writes?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked at a few V2 write nodes, but seems we don't have any SQL metrics there (even number of output rows). I guess we don't provide metrics for writes generally now? If there is interest to see metrics in writes, I think it is okay to work on it later.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like updating I think it would be good to follow up and support metrics on the output side. It doesn't need to be done here, but metrics are really useful.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me. I can work on it in follow up PRs. |
||
| val customMetrics = scan.supportedCustomMetrics().map { customMetric => | ||
| customMetric.getName -> SQLMetrics.createMetric(sparkContext, customMetric.getDescription) | ||
| } | ||
| Map("numOutputRows" -> SQLMetrics.createMetric(sparkContext, "number of output rows")) ++ | ||
| customMetrics | ||
| } | ||
|
|
||
| def scan: Scan | ||
|
|
||
|
|
||
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 think this should note that the metrics is a
long. What about calling it aLongMetric? Any plans to generalize it?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.
LongMetricsounds okay to me. This basically followsSQLMetricto use long value.