LuciferYang opened a new issue, #12861:
URL: https://github.com/apache/gluten/issues/12861
### 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.
--
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]
