Copilot commented on code in PR #12703:
URL: https://github.com/apache/gluten/pull/12703#discussion_r3861243767
##########
gluten-substrait/src/main/scala/org/apache/spark/sql/execution/GlutenExplainUtils.scala:
##########
@@ -45,6 +45,34 @@ import scala.collection.mutable.{ArrayBuffer, BitSet}
object GlutenExplainUtils extends AdaptiveSparkPlanHelper {
type FallbackInfo = (Int, Map[String, String])
+ /**
+ * Returns whether a plan should be ignored when collecting fallback
statistics.
+ *
+ * Such plans may be execution-framework or implementation wrappers.
+ */
+ def isFallbackInsensitivePlan(plan: SparkPlan): Boolean = plan match {
Review Comment:
The term “fallback insensitive” is a bit ambiguous—this helper is
effectively “ignore for fallback statistics” / “structural wrapper node.”
Consider renaming to something more directly descriptive (e.g.,
`shouldIgnoreInFallbackStats` or `isStructuralWrapperForFallbackStats`) to
reduce misinterpretation by future readers.
##########
gluten-substrait/src/main/scala/org/apache/spark/sql/execution/GlutenAutoAdjustStageResourceProfile.scala:
##########
@@ -145,8 +147,13 @@ case class
GlutenAutoAdjustStageResourceProfile(glutenConf: GlutenConfig, spark:
// case 2: check whether fallback exists and decide whether increase heap
memory
// and decrease offheap memory.
- val fallenNodeCnt = planNodes.count(p => !p.isInstanceOf[GlutenPlan])
- val totalCount = planNodes.size
+ val countedPlanNodes =
planNodes.filterNot(GlutenExplainUtils.isFallbackInsensitivePlan)
+ val fallenNodeCnt = countedPlanNodes.count {
+ case _: GlutenPlan => false
+ case i: InMemoryTableScanExec => !PlanUtil.isGlutenTableCache(i)
+ case _ => true
+ }
+ val totalCount = countedPlanNodes.size
if (1.0 * fallenNodeCnt / totalCount >=
glutenConf.autoAdjustStageFallenNodeThreshold) {
Review Comment:
`totalCount` can become `0` after filtering (e.g., for plans comprised only
of ignored/structural nodes like certain command/AQE wrappers). In that case
`fallenNodeCnt / totalCount` evaluates to `Infinity` and can incorrectly
trigger auto-adjustment. Add an explicit guard (e.g., require `totalCount > 0`
before computing the ratio, otherwise skip adjustment). Also consider adding a
UT for the `totalCount == 0` scenario to prevent regressions.
--
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]
