You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@gluten.apache.org by "zjuwangg (via GitHub)" <gi...@apache.org> on 2025/09/03 12:00:53 UTC

[PR] [GLUEN-10107][INFRA] Deprecate `isUseCelebornShuffleManager` from glutenConfig [incubator-gluten]

zjuwangg opened a new pull request, #10616:
URL: https://github.com/apache/incubator-gluten/pull/10616

   ## What changes are proposed in this pull request?
   * Introduce `NeedCustomShuffleWriterType` to let shuffle manager to decide shuffle writer type so that we can decouple `isUseCelebornShuffleManager` from GlutenConfig.
   * Remote unnecessary `isUseCelebornShuffleManager` in CH backend since corresponding code logic has guaranteed config value is always true. 
   
   ## How was this patch tested?
   
   Existing test
   


-- 
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-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [GLUEN-10107][INFRA] Deprecate `isUseCelebornShuffleManager` from glutenConfig [incubator-gluten]

Posted by "zjuwangg (via GitHub)" <gi...@apache.org>.

zjuwangg commented on code in PR #10616:
URL: https://github.com/apache/incubator-gluten/pull/10616#discussion_r2381074777


##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -553,30 +553,19 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
       partitioning: Partitioning,
       output: Seq[Attribute]): ShuffleWriterType = {
     val conf = GlutenConfig.get
-    // todo: remove isUseCelebornShuffleManager here
-    if (conf.isUseCelebornShuffleManager) {
-      if (conf.celebornShuffleWriterType == ReservedKeys.GLUTEN_SORT_SHUFFLE_WRITER) {
-        if (conf.useCelebornRssSort) {
-          RssSortShuffleWriterType
-        } else if (partitioning != SinglePartition) {
+    SparkEnv.get.shuffleManager match {
+      case shuffleManager: NeedCustomShuffleWriterType =>

Review Comment:
   @zhztheplayer or @PHILO-HE  can you help also take a look?



-- 
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-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [GLUEN-10107][INFRA] Deprecate `isUseCelebornShuffleManager` from glutenConfig [incubator-gluten]

Posted by "marin-ma (via GitHub)" <gi...@apache.org>.

marin-ma commented on code in PR #10616:
URL: https://github.com/apache/incubator-gluten/pull/10616#discussion_r2333732959


##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -553,30 +553,19 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
       partitioning: Partitioning,
       output: Seq[Attribute]): ShuffleWriterType = {
     val conf = GlutenConfig.get
-    // todo: remove isUseCelebornShuffleManager here
-    if (conf.isUseCelebornShuffleManager) {
-      if (conf.celebornShuffleWriterType == ReservedKeys.GLUTEN_SORT_SHUFFLE_WRITER) {
-        if (conf.useCelebornRssSort) {
-          RssSortShuffleWriterType
-        } else if (partitioning != SinglePartition) {
+    SparkEnv.get.shuffleManager match {
+      case shuffleManager: NeedCustomShuffleWriterType =>

Review Comment:
   In my opinion, the implementation of sort shuffle writer type for velox+celeborn is a special case. Perhaps we can add a function like `getShuffleWriterTypeForCeleborn` to the BackendsApi to handle this special case, and call it in the `getColumnarShuffleWriterType` in `CelebornShuffleManager`.



-- 
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-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [GLUEN-10107][INFRA] Deprecate `isUseCelebornShuffleManager` from glutenConfig [incubator-gluten]

Posted by "zjuwangg (via GitHub)" <gi...@apache.org>.

zjuwangg commented on PR #10616:
URL: https://github.com/apache/incubator-gluten/pull/10616#issuecomment-3270594790

   @zhztheplayer Would you like to help have a look of this refactor of config?


-- 
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-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org