[GLUTEN-12616][CORE] Guard SparkResourceUtil.getTaskSlots against non-positive task cpus by LuciferYang · Pull Request #12617 · apache/gluten

LuciferYang · 2026-07-24T05:40:32Z

What changes were proposed in this pull request?

SparkResourceUtil.getTaskSlots computed executorCores / taskCores with no guard, and callers divide by its result: GlutenDriverPlugin.setPredefinedConfigs does offHeapSize / taskSlots, and MemoryTargets, ColumnarShuffleWriter, and the Celeborn and Uniffle writers use it as a denominator too.

spark.task.cpus=0 made getTaskSlots itself throw ArithmeticException: / by zero. A negative value was worse: it produced a negative slot count, so per-task off-heap budgets came out negative and the driver started anyway. spark.task.cpus > spark.executor.cores made the integer division yield 0, so a caller's offHeapSize / taskSlots threw.

Gluten reads this value before Spark can reject it. getTaskSlots runs during PluginContainer init in SparkContext, before createTaskScheduler, and it reads the value with raw conf.getInt, which skips the ConfigEntry. Spark's CPUS_PER_TASK.checkValue(_ > 0) also only exists from Spark 4.2 (SPARK-55757); on 3.3 through 4.1 nothing validates it at all. There is no later Spark check to defer to, so getTaskSlots now fails fast with require(taskCores > 0, ...). Coercing the value to 1 would hide the misconfiguration and size every off-heap budget for a single slot.

The task.cpus > executor.cores case is handled differently. That is a cross-config relationship Spark validates with its own dedicated message (validateTaskCpusLargeEnough), so getTaskSlots floors the quotient at 1 to keep init from dividing by zero first, and leaves the reporting to Spark.

How was this patch tested?

Added SparkResourceUtilSuite with five tests: task.cpus=0 and task.cpus=-2 each fail with IllegalArgumentException naming the config, task.cpus greater than executor cores floors to one slot, 8 / 2 gives 4, and the default is one slot per core (asserted on local[8] so the default path is distinguishable). The three non-happy-path tests fail on the unfixed code and pass after the fix.

Closes #12616

…-positive task cpus getTaskSlots computed executorCores / taskCores with no guard. GlutenDriverPlugin.init reads the slot count and divides by it (and four other callers use it as a denominator), so two invalid configs crash there with an opaque ArithmeticException: - spark.task.cpus > spark.executor.cores makes the quotient 0, so a caller's offHeapSize / taskSlots throws. - spark.task.cpus = 0 makes getTaskSlots itself throw; it reads the value with raw conf.getInt, which bypasses Spark's CPUS_PER_TASK.checkValue(_ > 0). The plugin runs before createTaskScheduler, so Gluten throws before Spark's own validation (validateTaskCpusLargeEnough / CPUS_PER_TASK) can report the real misconfiguration with a clear message. Return a single slot for taskCores <= 0 and floor the quotient at 1 otherwise, deferring to Spark for the error text. Add SparkResourceUtilSuite covering both guarded branches and the normal paths.

github-actions · 2026-07-24T05:41:02Z

Run Gluten Clickhouse CI on x86

Copilot

Pull request overview

This PR hardens SparkResourceUtil.getTaskSlots so Gluten plugin initialization never triggers a divide-by-zero or returns 0 task slots for invalid spark.task.cpus settings, letting Spark’s own validation surface the proper user-facing error.

Changes:

Guard getTaskSlots against spark.task.cpus <= 0 by returning 1.
Floor executorCores / taskCores at 1 to avoid returning 0 when spark.task.cpus > executor cores.
Add a new SparkResourceUtilSuite covering the guarded behaviors and normal cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
gluten-core/src/main/scala/org/apache/spark/util/SparkResourceUtil.scala	Adds guards to prevent `/ by zero` and zero-slot results from invalid CPU-per-task configs.
gluten-core/src/test/scala/org/apache/spark/util/SparkResourceUtilSuite.scala	Introduces unit tests for `getTaskSlots` behavior across invalid and typical configurations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

LuciferYang · 2026-07-24T10:34:29Z

+  test("getTaskSlots returns one core per slot by default") {
+    val conf = new SparkConf(false).set("spark.master", "local[1]")
+    assert(SparkResourceUtil.getTaskSlots(conf) == 1)
+  }


Good catch. local[1] didn't actually distinguish the default behavior since 1 slot comes out regardless of the logic under test. Changed it to local[8] with spark.task.cpus unset, asserting 8 slots, so it now validates one-slot-per-core when task cpus defaults to 1.

…e master The default-behavior test used local[1], which yields 1 slot regardless of the logic under test. Use local[8] with spark.task.cpus unset and assert 8 slots, so it validates one-slot-per-core when task cpus defaults to 1.

github-actions · 2026-07-24T10:37:39Z

Run Gluten Clickhouse CI on x86

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

jackylee-ch · 2026-07-24T14:11:42Z

    val executorCores = SparkResourceUtil.getExecutorCores(conf)
    val taskCores = conf.getInt("spark.task.cpus", 1)
-    executorCores / taskCores
+    if (taskCores <= 0) {


the task cores has been checked while setting the configs, do we need this check again?

The check isn't redundant, for two reasons.

First, on the Spark versions Gluten targets, spark.task.cpus isn't validated at set time. The checkValue(_ > 0) on CPUS_PER_TASK was only added in SPARK-55757, which ships in Spark 4.2. I decompiled spark-core 3.5.5 to confirm: its CPUS_PER_TASK is ConfigBuilder("spark.task.cpus").version("0.5.0").intConf.createWithDefault(1), with no checkValue. So on Spark 3.3 through 4.1, nothing rejects a non-positive value before we read it.

Second, even on 4.2+ where the check exists, it only fires on a typed conf.get(CPUS_PER_TASK). The first such read is in SparkContext.createTaskScheduler, which runs after PluginContainer init. getTaskSlots is reached through that plugin init (GlutenDriverPlugin.init then setPredefinedConfigs), and it reads the value raw via conf.getInt("spark.task.cpus", 1), which skips the ConfigEntry. So we read the raw value before Spark validates it, on every version.

I verified this on a real driver init (new SparkContext with spark.plugins=org.apache.gluten.GlutenPlugin): spark.task.cpus=0 throws ArithmeticException: / by zero inside setPredefinedConfigs, and a negative value silently produces negative task slots and negative per-task off-heap budgets while the context still starts.

Based on your comment I switched the fix from coercing to 1 to failing fast with require(taskCores > 0, ...). A non-positive value is a real misconfiguration that should surface rather than be silently rewritten, which also matches the direction Spark took in 4.2. Just pushed the update, along with a refreshed PR description and issue rationale.

Coercing a non-positive spark.task.cpus to a single slot hid an invalid configuration. Spark treats a non-positive value as illegal (checkValue(_ > 0) on CPUS_PER_TASK, added in SPARK-55757 for 4.2+), so getTaskSlots should reject it rather than silently substitute 1. getTaskSlots reads spark.task.cpus via raw conf.getInt during driver plugin init, which bypasses Spark's checkValue and runs before Spark validates the value. On Spark < 4.2 that positivity check does not exist at all. Without a guard, a zero throws an opaque "/ by zero" ArithmeticException and a negative silently produces negative task slots and off-heap budgets. require(_ > 0) surfaces the misconfiguration with a clear message on every supported Spark version.

github-actions · 2026-07-27T10:28:31Z

Run Gluten Clickhouse CI on x86

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

+    // spark.task.cpus is read raw here, which bypasses Spark's own checkValue(_ > 0) (and on
+    // Spark < 4.2 that positivity check does not exist at all). getTaskSlots runs during driver
+    // plugin init, before Spark validates the value, so fail fast on a non-positive setting rather
+    // than dividing by it: a zero would throw an opaque "/ by zero" ArithmeticException and a
+    // negative would silently produce negative task slots and off-heap budgets.
    val taskCores = conf.getInt("spark.task.cpus", 1)
-    executorCores / taskCores
+    require(taskCores > 0, s"spark.task.cpus should be positive, but was $taskCores")


+  test("getTaskSlots fails fast when task cpus is zero") {
+    // spark.task.cpus is read via raw conf.getInt, which bypasses Spark's checkValue(_ > 0) (a
+    // check that only exists on Spark >= 4.2), so a zero value must not reach the division. Fail
+    // fast with a clear message instead of an opaque "/ by zero" ArithmeticException.
+    val conf = new SparkConf(false)
+      .set("spark.master", "local[8]")
+      .set("spark.task.cpus", "0")
+    val e = intercept[IllegalArgumentException](SparkResourceUtil.getTaskSlots(conf))
+    assert(e.getMessage.contains("spark.task.cpus should be positive"))
+  }


jackylee-ch

Good catch.

LuciferYang · 2026-07-28T02:32:58Z

Thank you @jackylee-ch

Copilot AI review requested due to automatic review settings July 24, 2026 05:40

github-actions Bot added the CORE works for Gluten Core label Jul 24, 2026

Copilot started reviewing on behalf of LuciferYang July 24, 2026 05:40 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 24, 2026 10:34

Copilot started reviewing on behalf of LuciferYang July 24, 2026 10:37 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

jackylee-ch reviewed Jul 24, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 27, 2026 10:27

Copilot started reviewing on behalf of LuciferYang July 27, 2026 10:28 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

jackylee-ch approved these changes Jul 27, 2026

View reviewed changes

jackylee-ch merged commit 40dc792 into apache:main Jul 28, 2026
56 checks passed

Navigation Menu

Uh oh!

[GLUTEN-12616][CORE] Guard SparkResourceUtil.getTaskSlots against non-positive task cpus - #12617

[GLUTEN-12616][CORE] Guard SparkResourceUtil.getTaskSlots against non-positive task cpus#12617
jackylee-ch merged 3 commits into
apache:mainfrom
LuciferYang:fix/task-slots-divide-by-zero

LuciferYang commented Jul 24, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

LuciferYang Jul 24, 2026

Uh oh!

github-actions Bot commented Jul 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

jackylee-ch Jul 24, 2026

Uh oh!

LuciferYang Jul 27, 2026

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

jackylee-ch left a comment

Uh oh!

Uh oh!

LuciferYang commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

LuciferYang commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

github-actions Bot commented Jul 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

LuciferYang Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

jackylee-ch Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

LuciferYang Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

jackylee-ch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LuciferYang commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LuciferYang commented Jul 24, 2026 •

edited

Loading