gluten-ut: GlutenCachedTableSuite silently loses spark.sql.shuffle.partitions=5
- 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
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