zjuwangg commented on code in PR #8195:
URL: https://github.com/apache/incubator-gluten/pull/8195#discussion_r1881812793
##########
backends-clickhouse/src/main/scala/org/apache/gluten/backendsapi/clickhouse/CHSparkPlanExecApi.scala:
##########
@@ -469,7 +470,8 @@ class CHSparkPlanExecApi extends SparkPlanExecApi with
Logging {
mode: BroadcastMode,
child: SparkPlan,
numOutputRows: SQLMetric,
- dataSize: SQLMetric): BuildSideRelation = {
+ dataSize: SQLMetric,
+ resourceProfile: Option[ResourceProfile] = None): BuildSideRelation = {
Review Comment:
Not so.
IMO `createBroadcastRelation` used in BroadcastExchangeExec, both velox and
ch backends can through this this interface to setResourceProfile
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/WholeStageTransformer.scala:
##########
@@ -194,10 +195,22 @@ trait UnaryTransformSupport extends TransformSupport with
UnaryExecNode {
}
}
+/** Base interface for a query plan that can be used to set ResourceProfile. */
+trait WithResourceProfileSupport {
+ private var resourceProfile: Option[ResourceProfile] = None
Review Comment:
Good points. I will try in this way!
##########
gluten-substrait/src/main/scala/org/apache/spark/sql/execution/ColumnarShuffleExchangeExec.scala:
##########
@@ -161,6 +162,10 @@ case class ColumnarShuffleExchangeExec(
override def doExecuteColumnar(): RDD[ColumnarBatch] = {
if (cachedShuffleRDD == null) {
cachedShuffleRDD = new
ShuffledColumnarBatchRDD(columnarShuffleDependency, readMetrics)
+ if (getResourceProfile.isDefined) {
+ log.info(s"Set resource profile for $child to
${getResourceProfile.get}")
Review Comment:
Will address it soon.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/WholeStageTransformer.scala:
##########
@@ -194,10 +195,22 @@ trait UnaryTransformSupport extends TransformSupport with
UnaryExecNode {
}
}
+/** Base interface for a query plan that can be used to set ResourceProfile. */
+trait WithResourceProfileSupport {
+ private var resourceProfile: Option[ResourceProfile] = None
+
+ def withResourceProfile(resourceProfile: ResourceProfile): Unit = {
Review Comment:
It make sense, will address it.
##########
backends-clickhouse/src/main/scala/org/apache/gluten/backendsapi/clickhouse/CHSparkPlanExecApi.scala:
##########
@@ -469,7 +470,8 @@ class CHSparkPlanExecApi extends SparkPlanExecApi with
Logging {
mode: BroadcastMode,
child: SparkPlan,
numOutputRows: SQLMetric,
- dataSize: SQLMetric): BuildSideRelation = {
+ dataSize: SQLMetric,
+ resourceProfile: Option[ResourceProfile] = None): BuildSideRelation = {
Review Comment:
In CH backend share the same `ColumnarShuffleExchangeExec` as Velox, so we
also can set ResourceProfile direct in `ColumnarShuffleExchangeExec` in CH.
That means the behavior is consistent.
Correct me if I am wrong
##########
backends-clickhouse/src/main/scala/org/apache/gluten/backendsapi/clickhouse/CHSparkPlanExecApi.scala:
##########
@@ -469,7 +470,8 @@ class CHSparkPlanExecApi extends SparkPlanExecApi with
Logging {
mode: BroadcastMode,
child: SparkPlan,
numOutputRows: SQLMetric,
- dataSize: SQLMetric): BuildSideRelation = {
+ dataSize: SQLMetric,
+ resourceProfile: Option[ResourceProfile] = None): BuildSideRelation = {
Review Comment:
Or you mean I should do the underlying implementation？
I'm not so familiar with ch backend but I can try or add a todo here if you
mean this.
--
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: commits-unsubscr...@gluten.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@gluten.apache.org
For additional commands, e-mail: commits-h...@gluten.apache.org
