Copilot commented on code in PR #13072: URL: https://github.com/apache/gluten/pull/13072#discussion_r4079136710
########## gluten-substrait/src/main/scala/org/apache/spark/shuffle/sort/ColumnarShuffleManager.scala: ########## @@ -51,11 +57,27 @@ class ColumnarShuffleManager(conf: SparkConf) import ColumnarShuffleManager._ private lazy val shuffleExecutorComponents = loadShuffleExecutorComponents(conf) - override val shuffleBlockResolver = new IndexShuffleBlockResolver(conf) - /** A mapping from shuffle ids to the number of mappers producing output for those shuffles. */ + /** + * A mapping from shuffle ids to the task ids of mappers producing output for those shuffles. + * + * Must be declared before `shuffleBlockResolver`: Scala initializes vals in declaration order, so + * the resolver would otherwise capture `null`. + */ private[this] val taskIdMapsForShuffle = new ConcurrentHashMap[Int, OpenHashSet[Long]]() + // Mirrors SortShuffleManager: the resolver must share this map rather than allocate its own. It + // records blocks migrated in during executor decommissioning, and `unregisterShuffle` reads the + // same map to delete the corresponding map output. + // + // The argument is positional rather than named because the constructor signature differs across + // supported Spark versions: 3.4 and 3.5 default both `_blockManager` and `taskIdMapsForShuffle`, + // 4.0 drops the defaults, and 4.1 narrows the map type from `java.util.Map` to + // `java.util.concurrent.ConcurrentMap`. The positional form compiles against all of them, but it + // is signature-sensitive -- re-verify it when adding a new Spark version. + override val shuffleBlockResolver = + new IndexShuffleBlockResolver(conf, null, taskIdMapsForShuffle) Review Comment: The second positional argument is the resolver's `_blockManager`; passing `null` disables its default and leaves the resolver with a null block manager. When `unregisterShuffle` calls `removeDataByMap` for a migrated map, the resolver needs `blockManager.diskBlockManager` to locate/delete the files and will fail instead of cleaning them up. Pass the same block manager that the single-argument constructor would resolve. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
