apache / apache/gluten

gluten-ut: GlutenCachedTableSuite silently loses spark.sql.shuffle.partitions=5

Open
#12,861 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Scala
Stars
1.6k
Forks
657
Avg merge
2d 14h
Merged PRs (30d)
80

Description

### What happens

`GlutenCachedTableSuite.sparkConf` is two statements, and the first one's result is thrown away. In `gluten-ut/spark35/src/test/scala/org/apache/spark/sql/GlutenCachedTableSuite.scala:36`:

```scala
override def sparkConf: SparkConf = {
super.sparkConf.set("spark.sql.shuffle.partitions", "5")
super.sparkConf.set(GlutenConfig.COLUMNAR_TABLE_CACHE_ENABLED.key, "true")
}
```

`super.sparkConf` is a `def`: `SharedSparkSessionBase.sparkConf` builds a fresh `SparkConf` on every call. So the first line creates a conf, sets `spark.sql.shuffle.partitions=5` on it, and drops it on the floor. The method returns the second conf, which never saw that setting, and the suite runs with the `spark.sql.shuffle.partitions=1` that `GlutenSQLTestsBaseTrait.nativeSparkConf` puts there.

Same code in all five version modules.

### Why it matters

The suite silently runs under a different shuffle configuration than it asks for. Anything in it that depends on partition count, and cached-relation partitioning is exactly that kind of thing, is testing a shape nobody intended. It also reads as correct, which is why it has survived: you have to know `sparkConf` is a `def` and not a `val` to see the bug.

The line right above it, `sys.props.put(GlutenConfig.COLUMNAR_TABLE_CACHE_ENABLED.key, "true")` at `:35`, is a separate small problem. It writes a JVM-wide system property that nothing restores, and its comment says "for temporarily disable the columnar table cache globally" while the value it sets is `true`. The conf already defaults to `true`, so nothing behaves differently, but the line is misleading and the leak is real.

### Suggested fix

Chain the calls:

```scala
override def sparkConf: SparkConf =
super.sparkConf
.set("spark.sql.shuffle.partitions", "5")
.set(GlutenConfig.COLUMNAR_TABLE_CACHE_ENABLED.key, "true")
```

Then run the suite: it has never actually executed with 5 shuffle partitions, so some expectations may need adjusting. Drop the `sys.props` line in the same change, and fix or delete the comment above it.

Found during a review pass on #12840, which touched a different suite with the same `sys.props` idiom.

Contributor guide

Open the contributing guide

Research direction

Start with gluten-ut/spark35/src/test/scala/org/apache/spark/sql/GlutenCachedTableSuite.scala and compare the corresponding suite files in all five version modules. Run GlutenCachedTableSuite before and after chaining the SparkConf calls, then verify that the suite uses five shuffle partitions, the misleading sys.props line and comment are corrected, and the tests pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
scala
Domain
backend, testing
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.