[GLUTEN-8018][CORE] Introduce ApplyResourceProfileExec to apply resource profile for query stage #8195

zjuwangg · 2024-12-10T06:48:13Z

What changes were proposed in this pull request?

This PR aims to add interface to GlutenPlan so that we can have the chance to set resource profile,
detailed design can be found #8018.

How was this patch tested?

To be added in the following commit.

github-actions · 2024-12-10T06:48:30Z

#8018

github-actions · 2024-12-10T06:48:44Z

Run Gluten Clickhouse CI on x86

zjuwangg · 2024-12-11T08:51:16Z

#8209 is a follow up PR and demonstrate how the rule work with PR, please help review it when your guys have time. 🙏🏻

@zhztheplayer @FelixYBW @Yohahaha @PHILO-HE @jackylee-ch

gluten-substrait/src/main/scala/org/apache/gluten/execution/WholeStageTransformer.scala

...-substrait/src/main/scala/org/apache/spark/sql/execution/ColumnarBroadcastExchangeExec.scala

...-clickhouse/src/main/scala/org/apache/gluten/backendsapi/clickhouse/CHSparkPlanExecApi.scala

gluten-substrait/src/main/scala/org/apache/gluten/execution/WholeStageTransformer.scala

...en-substrait/src/main/scala/org/apache/spark/sql/execution/ColumnarShuffleExchangeExec.scala

zjuwangg

@zhztheplayer Thanks for your detailed review. I will address your comments soon.

...-clickhouse/src/main/scala/org/apache/gluten/backendsapi/clickhouse/CHSparkPlanExecApi.scala

gluten-substrait/src/main/scala/org/apache/gluten/execution/WholeStageTransformer.scala

...en-substrait/src/main/scala/org/apache/spark/sql/execution/ColumnarShuffleExchangeExec.scala

zhztheplayer · 2024-12-13T02:47:52Z

@zjuwangg Aside of the comments, I am thinking of another possible solution. Do you think we can use a rule to add a dummy unary query plan node on the input side of exchanges, to change the input RDD's query profile?

Which means, a plan

ColumnarExchange
  WholeStageTransformer
    ProjectExecTransformer
      FilterExecTransformer
        ScanExecTransformer

Can become

ColumnarExchange
  AdjustResourceProfile (or Apply/AdjustMemoryResources or something)
    WholeStageTransformer
      ProjectExecTransformer
        FilterExecTransformer
          ScanExecTransformer

When the rule applies.

Once AdjustResourceProfile's doExecute / doExecuteColumnar is called, we can set the resource profile to it's child RDD.

I feel this way is more intuitive to user and could make less code impact to other operators' code. What do you think?

zjuwangg · 2024-12-13T03:21:37Z

@zjuwangg Aside of the comments, I am thinking of another possible solution. Do you think we can use a rule to add a dummy unary query plan node on the input side of exchanges, to change the input RDD's query profile?

Which means, a plan
ColumnarExchange
  WholeStageTransformer
    ProjectExecTransformer
      FilterExecTransformer
        ScanExecTransformer
Can become
ColumnarExchange
  AdjustResourceProfile (or Apply/AdjustMemoryResources or something)
    WholeStageTransformer
      ProjectExecTransformer
        FilterExecTransformer
          ScanExecTransformer
When the rule applies.

Once AdjustResourceProfile's doExecute / doExecuteColumnar is called, we can set the resource profile to it's child RDD.

I feel this way is more intuitive to user and could make less code impact to other operators' code. What do you think?

@zhztheplayer It's truely possible. Since I now do in this way for the whole stage fallback vanilla spark node in

incubator-gluten/gluten-substrait/src/main/scala/org/apache/spark/sql/execution/GlutenAutoAdjustStageResourceProfile.scala

Lines 184 to 205 in 8073c47

    
           /** Used to wrap a row-based child plan to have a change to setting ResourceProfile. */ 
        
           case class RowBasedNodeResourceProfileWrapperExec( 
        
               child: SparkPlan, 
        
               resourceProfile: Option[ResourceProfile]) 
        
             extends UnaryExecNode { 
        
             if (child.logicalLink.isDefined) { 
        
               setLogicalLink(RowBasedNodeResourProfileWrapperExecAdaptor(child.logicalLink.get)) 
        
             } 
        
             override def output: Seq[Attribute] = child.output 
        
             override protected def doExecute(): RDD[InternalRow] = { 
        
               if (resourceProfile.isDefined) { 
        
                 log.info(s"Use resource profile ${resourceProfile.get} for RowOperatorWrapperExec.") 
        
                 child.execute.withResources(resourceProfile.get) 
        
               } else { 
        
                 log.info(s"resourceProfile is empty in RowOperatorWrapperExec!") 
        
                 child.execute 
        
               } 
        
             }

And the plan will be like following

I feel this way is more intuitive to user and could make less code impact to other operators' code. What do you think?

Good insights!
It's more elegant and can also work on whole fallback stage , will try to refactor in this way!

github-actions · 2024-12-13T08:41:28Z

Run Gluten Clickhouse CI on x86

zjuwangg · 2024-12-13T08:46:05Z

@zhztheplayer I just revert previous commit and introduce a new ApplyResourceProfileExec node. Plz have a look when you have time.

zhztheplayer

Would you also please fix CI error? Thanks!

gluten-substrait/src/main/scala/org/apache/gluten/execution/ApplyResourceProfileExec.scala

...-clickhouse/src/main/scala/org/apache/gluten/backendsapi/clickhouse/CHSparkPlanExecApi.scala

jackylee-ch · 2024-12-16T03:14:42Z

gluten-substrait/src/main/scala/org/apache/gluten/execution/ApplyResourceProfileExec.scala

+ * @param child
+ * @param resourceProfile
+ */
+case class ApplyResourceProfileExec(child: SparkPlan, resourceProfile: ResourceProfile)


Maybe add a suite case to show how this works?
The title of this pr should be changed now.

Maybe add a suite case to show how this works? The title of this pr should be changed now.

Right. Will change the title and address these comments.

@jackylee-ch Thanks for your detailed review, I'll add a test suite in a following MR after this get merged and just change the tile to match commit.

@zjuwangg Would you add @Experimental to ApplyResourceProfileExec if it will be tested in the next PR? Thanks.

github-actions · 2024-12-16T13:58:29Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-16T13:59:56Z

Run Gluten Clickhouse CI on x86

FelixYBW · 2024-12-16T22:07:50Z

e whole stage fallback vanilla spark n

Thank you, can you put a chart of query plan again?

zhztheplayer · 2024-12-17T00:08:58Z

gluten-substrait/src/main/scala/org/apache/spark/sql/execution/ApplyResourceProfileExec.scala

+  if (child.logicalLink.isDefined) {
+    setLogicalLink(ApplyResourceProfileExecAdaptor(child.logicalLink.get))
+  }


Missed a question here. Is there specific reason of setting the logical link with an ApplyResourceProfileExecAdaptor?

Actually no. Just keep consistent with FakeRowLogicAdaptor.

Actually no. Just keep consistent with FakeRowLogicAdaptor.

I see. that one may have specific reason, I am not sure.

So can we remove ApplyResourceProfileExecAdaptor? I feel ApplyResourceProfileExecAdaptor's logical link should be the same with its child's logical link. Since it and its child seem to be logically equivalent (share the same schema, output, statistics, etc.). Thanks.

zjuwangg · 2024-12-17T03:20:05Z

e whole stage fallback vanilla spark n

Thank you, can you put a chart of query plan again?

@FelixYBW
Similar to this, I will update it when following MR get ready

zjuwangg · 2024-12-17T05:48:26Z

Run Gluten Clickhouse CI on x86

zjuwangg · 2024-12-17T05:59:21Z

@zhztheplayer @PHILO-HE do you know how to trigger the falling test again? The failure seems all related Network problem.

zhztheplayer · 2024-12-17T06:02:10Z

do you know how to trigger the falling test again?

@zjuwangg I know there is a way to retrigger CI by amending an empty commit and push it:

git commit --allow-empty -m "trigger ci"
git push ...

github-actions · 2024-12-17T06:19:30Z

Run Gluten Clickhouse CI on x86

…stage

github-actions · 2024-12-17T06:47:03Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-17T07:25:49Z

Run Gluten Clickhouse CI on x86

zhztheplayer

Now the code feels independent enough to merge with an @Experimental flag added. +1 from my side. We can move to #8209 then.

CC @jackylee-ch If any further comments.

jackylee-ch · 2024-12-17T08:35:44Z

Now the code feels independent enough to merge with an @Experimental flag added.

Also +1 for me, thanks.

github-actions bot added CORE works for Gluten Core VELOX CLICKHOUSE labels Dec 10, 2024

This was referenced Dec 10, 2024

[CORE] Implement stage-level resourceProfile auto-adjust framework to avoid oom #8018

Open

[GLUTEN-8018][VL] Support adjusting stage resource profile dynamically #8209

Open

zhztheplayer changed the title ~~[GLUTEN-8018][CORE]Add setResourceProfile interface to provided ability to set resourceProfile~~ [GLUTEN-8018][CORE] Add setResourceProfile interface to provided ability to set resourceProfile Dec 12, 2024

zhztheplayer reviewed Dec 12, 2024

View reviewed changes

zjuwangg commented Dec 12, 2024

View reviewed changes

github-actions bot removed VELOX CLICKHOUSE labels Dec 13, 2024

zhztheplayer approved these changes Dec 16, 2024

View reviewed changes

gluten-substrait/src/main/scala/org/apache/gluten/execution/ApplyResourceProfileExec.scala Outdated Show resolved Hide resolved

gluten-substrait/src/main/scala/org/apache/gluten/execution/ApplyResourceProfileExec.scala Outdated Show resolved Hide resolved

jackylee-ch reviewed Dec 16, 2024

View reviewed changes

zjuwangg force-pushed the m_stageResourceProfile branch from 6bfa7e6 to 25a2d4d Compare December 16, 2024 13:57

zjuwangg force-pushed the m_stageResourceProfile branch from 25a2d4d to c792d03 Compare December 16, 2024 13:59

zjuwangg changed the title ~~[GLUTEN-8018][CORE] Add setResourceProfile interface to provided ability to set resourceProfile~~ [GLUTEN-8018][CORE] Introduce ApplyResourceProfileExec to apply resource profile for the stage Dec 16, 2024

zhztheplayer reviewed Dec 17, 2024

View reviewed changes

PHILO-HE changed the title ~~[GLUTEN-8018][CORE] Introduce ApplyResourceProfileExec to apply resource profile for the stage~~ [GLUTEN-8018][CORE] Introduce ApplyResourceProfileExec to apply resource profile for query stage Dec 17, 2024

zjuwangg added 3 commits December 17, 2024 14:46

Introduce ApplyResourceProfileExec to apply resource profile for the …

c9260da

…stage

remove logical adaptor

58e7535

trigger ci

1376cfb

zjuwangg force-pushed the m_stageResourceProfile branch from 3c9aa29 to 1376cfb Compare December 17, 2024 06:46

add Experimental marker

7e78c90

zhztheplayer approved these changes Dec 17, 2024

View reviewed changes

jackylee-ch approved these changes Dec 17, 2024

View reviewed changes

zhztheplayer merged commit e94e01d into apache:main Dec 17, 2024
43 of 45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GLUTEN-8018][CORE] Introduce ApplyResourceProfileExec to apply resource profile for query stage #8195

[GLUTEN-8018][CORE] Introduce ApplyResourceProfileExec to apply resource profile for query stage #8195

zjuwangg commented Dec 10, 2024

github-actions bot commented Dec 10, 2024

github-actions bot commented Dec 10, 2024

zjuwangg commented Dec 11, 2024

zjuwangg left a comment

zhztheplayer commented Dec 13, 2024 •

edited

Loading

zjuwangg commented Dec 13, 2024

github-actions bot commented Dec 13, 2024

zjuwangg commented Dec 13, 2024

zhztheplayer left a comment

jackylee-ch Dec 16, 2024

zjuwangg Dec 16, 2024 •

edited

Loading

zjuwangg Dec 16, 2024

zhztheplayer Dec 17, 2024

zjuwangg Dec 17, 2024

github-actions bot commented Dec 16, 2024

github-actions bot commented Dec 16, 2024

FelixYBW commented Dec 16, 2024

zhztheplayer Dec 17, 2024

zjuwangg Dec 17, 2024

zhztheplayer Dec 17, 2024

zjuwangg Dec 17, 2024

zjuwangg commented Dec 17, 2024 •

edited

Loading

zjuwangg commented Dec 17, 2024

zjuwangg commented Dec 17, 2024

zhztheplayer commented Dec 17, 2024 •

edited

Loading

github-actions bot commented Dec 17, 2024

github-actions bot commented Dec 17, 2024

github-actions bot commented Dec 17, 2024

zhztheplayer left a comment

jackylee-ch commented Dec 17, 2024

[GLUTEN-8018][CORE] Introduce ApplyResourceProfileExec to apply resource profile for query stage #8195

[GLUTEN-8018][CORE] Introduce ApplyResourceProfileExec to apply resource profile for query stage #8195

Conversation

zjuwangg commented Dec 10, 2024

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented Dec 10, 2024

github-actions bot commented Dec 10, 2024

zjuwangg commented Dec 11, 2024

zjuwangg left a comment

Choose a reason for hiding this comment

zhztheplayer commented Dec 13, 2024 • edited Loading

zjuwangg commented Dec 13, 2024

github-actions bot commented Dec 13, 2024

zjuwangg commented Dec 13, 2024

zhztheplayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjuwangg Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 16, 2024

github-actions bot commented Dec 16, 2024

FelixYBW commented Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjuwangg commented Dec 17, 2024 • edited Loading

zjuwangg commented Dec 17, 2024

zjuwangg commented Dec 17, 2024

zhztheplayer commented Dec 17, 2024 • edited Loading

github-actions bot commented Dec 17, 2024

github-actions bot commented Dec 17, 2024

github-actions bot commented Dec 17, 2024

zhztheplayer left a comment

Choose a reason for hiding this comment

jackylee-ch commented Dec 17, 2024

zhztheplayer commented Dec 13, 2024 •

edited

Loading

zjuwangg Dec 16, 2024 •

edited

Loading

zjuwangg commented Dec 17, 2024 •

edited

Loading

zhztheplayer commented Dec 17, 2024 •

edited

Loading