[GitHub] [spark] ulysses-you commented on a change in pull request #35719: [SPARK-38401][SQL][CORE] Unify get preferred locations for shuffle in AQE

[GitBox <gi...@apache.org>]

ulysses-you commented on a change in pull request #35719:
URL: https://github.com/apache/spark/pull/35719#discussion_r819326181



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/ShuffledRowRDD.scala
##########
@@ -177,19 +177,36 @@ class ShuffledRowRDD(
     val tracker = SparkEnv.get.mapOutputTracker.asInstanceOf[MapOutputTrackerMaster]
     partition.asInstanceOf[ShuffledRowRDDPartition].spec match {
       case CoalescedPartitionSpec(startReducerIndex, endReducerIndex, _) =>
-        // TODO order by partition size.
-        startReducerIndex.until(endReducerIndex).flatMap { reducerIndex =>
-          tracker.getPreferredLocationsForShuffle(dependency, reducerIndex)
-        }
+        tracker.getPreferredLocationsForShuffle(
+          dependency,
+          0,
+          dependency.rdd.getNumPartitions,
+          startReducerIndex,
+          endReducerIndex)
 
-      case PartialReducerPartitionSpec(_, startMapIndex, endMapIndex, _) =>
-        tracker.getMapLocation(dependency, startMapIndex, endMapIndex)
+      case PartialReducerPartitionSpec(reducerIndex, startMapIndex, endMapIndex, _) =>
+        tracker.getPreferredLocationsForShuffle(

Review comment:
       According to the original idea of preferred locations. It's more effective to schedule task to a bigger data size node. And same explanation with the comment of the threshold `REDUCER_PREF_LOCS_FRACTION`.
   ```scala
   // Fraction of total map output that must be at a location for it to considered as a preferred
   // location for a reduce task. Making this larger will focus on fewer locations where most data
   // can be read locally, but may lead to more delay in scheduling if those locations are busy.
   ```
   
   If return locations of all blocks, the preferred locations will be less meaning since the location can acutally contain less data and we can only reduce a little data size with network traffic. One more probleam, store all locations at driver side will cause OOM, and it get worser if we allocate more executors.
   
   So this pr just correct the behavior of preferred locations. But if you think it's worth to return more locations, we can expose the `REDUCER_PREF_LOCS_FRACTION` as a config (I think it should be another thread ?).
   
   Hope it makes sense for you.




-- 
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: reviews-unsubscribe@spark.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org