Skip to content

Commit d2b48f1

Browse files
malinjawiMohammad Linjawi
malinjawi
and
Mohammad Linjawi
authored
[GLUTEN-11998][CORE] Fix incorrect modifiability status for GlutenConfig entries (#12036)
This PR fixes GlutenConfig runtime modifiability reporting for #11998. GlutenConfig.get previously constructed GlutenConfig from SQLConf.get, the JVM thread-local SQLConf. Spark treats configs stored only in that static/thread-local SQLConf path as non-modifiable, so spark.conf.isModifiable("spark.gluten.*") could report false even for Gluten configs that are not static. Main changes: register Gluten config entries with Spark SQLConf metadata preserve static Gluten config behavior by registering static configs through SQLConf.buildStaticConf register config alternatives as SQLConf entries as well update GlutenCoreConfig.get and GlutenConfig.get to read the active SparkSession SQLConf before falling back to SQLConf.get add focused runtime config coverage for the exact SparkSession API path: spark.conf.isModifiable(...) add focused runtime config coverage proving GlutenConfig.get reads active SparkSession config changes Co-authored-by: Mohammad Linjawi <Mohammad.Linjawi@ibm.com>
1 parent 7bb00d1 commit d2b48f1

12 files changed

Lines changed: 408 additions & 249 deletions

File tree

‎backends-velox/src/test/scala/org/apache/gluten/config/AllVeloxConfiguration.scala‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,8 @@ class AllVeloxConfiguration extends AnyFunSuite {
4242
s"""
4343
|## Gluten Velox backend configurations
4444
|
45-
| Key | Default | Description
46-
| --- | --- | ---
45+
| Key | Modifiability | Default | Description
46+
| --- | --- | --- | ---
4747
|"""
4848

4949
VeloxConfig.allEntries
@@ -53,16 +53,20 @@ class AllVeloxConfiguration extends AnyFunSuite {
5353
.foreach {
5454
entry =>
5555
val dft = entry.defaultValueString.replace("<", "&lt;").replace(">", "&gt;")
56-
builder += Seq(s"${entry.key}", s"$dft", s"${entry.doc}")
56+
builder += Seq(
57+
s"${entry.key}",
58+
AllGlutenConfiguration.configModifiability(entry.key),
59+
s"$dft",
60+
s"${entry.doc}")
5761
.mkString("|")
5862
}
5963

6064
builder ++=
6165
s"""
6266
|## Gluten Velox backend *experimental* configurations
6367
|
64-
| Key | Default | Description
65-
| --- | --- | ---
68+
| Key | Modifiability | Default | Description
69+
| --- | --- | --- | ---
6670
|"""
6771

6872
VeloxConfig.allEntries
@@ -72,7 +76,11 @@ class AllVeloxConfiguration extends AnyFunSuite {
7276
.foreach {
7377
entry =>
7478
val dft = entry.defaultValueString.replace("<", "&lt;").replace(">", "&gt;")
75-
builder += Seq(s"${entry.key}", s"$dft", s"${entry.doc}")
79+
builder += Seq(
80+
s"${entry.key}",
81+
AllGlutenConfiguration.configModifiability(entry.key),
82+
s"$dft",
83+
s"${entry.doc}")
7684
.mkString("|")
7785
}
7886

‎docs/Configuration.md‎

Lines changed: 145 additions & 145 deletions
Large diffs are not rendered by default.

‎docs/velox-configuration.md‎

Lines changed: 80 additions & 80 deletions
Large diffs are not rendered by default.

‎gluten-core/src/main/scala/org/apache/gluten/config/ConfigRegistry.scala‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,35 @@ trait ConfigRegistry {
2929
require(existing.isEmpty, s"Config entry ${entry.key} already registered!")
3030
}
3131

32+
private def registerToSQLConf(entry: ConfigEntry[_], isStatic: Boolean): Unit = {
33+
(entry.key :: entry.alternatives).foreach(registerToSQLConf(entry, _, isStatic))
34+
}
35+
36+
private def registerToSQLConf(entry: ConfigEntry[_], key: String, isStatic: Boolean): Unit = {
37+
var builder =
38+
if (isStatic) SQLConf.buildStaticConf(key) else SQLConf.buildConf(key)
39+
if (entry.doc.nonEmpty) {
40+
builder = builder.doc(entry.doc)
41+
}
42+
if (entry.version.nonEmpty) {
43+
builder = builder.version(entry.version)
44+
}
45+
if (!entry.isPublic) {
46+
builder = builder.internal()
47+
}
48+
49+
val sparkEntry = builder.stringConf.transform {
50+
value =>
51+
entry.valueConverter(value)
52+
value
53+
}
54+
if (entry.defaultValue.isDefined) {
55+
sparkEntry.createWithDefaultString(entry.defaultValueString)
56+
} else {
57+
sparkEntry.createOptional
58+
}
59+
}
60+
3261
/** Visible for testing. */
3362
private[config] def allEntries: Seq[ConfigEntry[_]] = {
3463
configEntries.values.toSeq
@@ -38,15 +67,16 @@ trait ConfigRegistry {
3867
ConfigBuilder(key).onCreate {
3968
entry =>
4069
register(entry)
70+
registerToSQLConf(entry, isStatic = false)
4171
ConfigRegistry.registerToAllEntries(entry)
4272
}
4373
}
4474

4575
protected def buildStaticConf(key: String): ConfigBuilder = {
4676
ConfigBuilder(key).onCreate {
4777
entry =>
48-
SQLConf.registerStaticConfigKey(key)
4978
register(entry)
79+
registerToSQLConf(entry, isStatic = true)
5080
ConfigRegistry.registerToAllEntries(entry)
5181
}
5282
}

‎gluten-core/src/main/scala/org/apache/gluten/config/GlutenCoreConfig.scala‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package org.apache.gluten.config
1818

1919
import org.apache.spark.internal.Logging
2020
import org.apache.spark.network.util.ByteUnit
21+
import org.apache.spark.sql.SparkSession
2122
import org.apache.spark.sql.internal.{SQLConf, SQLConfProvider}
2223

2324
class GlutenCoreConfig(conf: SQLConf) extends Logging {
@@ -62,7 +63,14 @@ class GlutenCoreConfig(conf: SQLConf) extends Logging {
6263
*/
6364
object GlutenCoreConfig extends ConfigRegistry {
6465
override def get: GlutenCoreConfig = {
65-
new GlutenCoreConfig(SQLConf.get)
66+
new GlutenCoreConfig(activeSQLConf)
67+
}
68+
69+
private[gluten] def activeSQLConf: SQLConf = {
70+
SparkSession.getActiveSession
71+
.filterNot(_.sparkContext.isStopped)
72+
.map(_.sessionState.conf)
73+
.getOrElse(SQLConf.get)
6674
}
6775

6876
val SPARK_OFFHEAP_SIZE_KEY = "spark.memory.offHeap.size"

‎gluten-substrait/src/main/scala/org/apache/gluten/config/GlutenConfig.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,7 @@ object GlutenConfig extends ConfigRegistry {
465465
val SPARK_MAX_BROADCAST_TABLE_SIZE = "spark.sql.maxBroadcastTableSize"
466466

467467
def get: GlutenConfig = {
468-
new GlutenConfig(SQLConf.get)
468+
new GlutenConfig(GlutenCoreConfig.activeSQLConf)
469469
}
470470

471471
def prefixOf(backendName: String): String = s"spark.gluten.sql.columnar.backend.$backendName"

‎gluten-substrait/src/test/scala/org/apache/gluten/config/AllGlutenConfiguration.scala‎

Lines changed: 48 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
*/
1717
package org.apache.gluten.config
1818

19+
import org.apache.spark.sql.internal.SQLConf
20+
1921
import org.scalactic.Prettifier
2022
import org.scalactic.source.Position
2123
import org.scalatest.Assertions._
@@ -67,44 +69,55 @@ class AllGlutenConfiguration extends AnyFunSuite {
6769
s"""
6870
|## Spark base configurations for Gluten plugin
6971
|
70-
| Key | Recommend Setting | Description
71-
| --- | --- | ---
72+
| Key | Modifiability | Recommend Setting | Description
73+
| --- | --- | --- | ---
7274
|"""
7375

7476
// scalastyle:off
7577
builder += Seq(
7678
"spark.plugins",
79+
AllGlutenConfiguration.configModifiability("spark.plugins"),
7780
"org.apache.gluten.GlutenPlugin",
78-
"To load Gluten's components by Spark's plug-in loader.").mkString("|")
81+
"To load Gluten's components by Spark's plug-in loader."
82+
).mkString("|")
7983
builder += Seq(
8084
"spark.memory.offHeap.enabled",
85+
AllGlutenConfiguration.configModifiability("spark.memory.offHeap.enabled"),
8186
"true",
82-
"Gluten use off-heap memory for certain operations.").mkString("|")
87+
"Gluten use off-heap memory for certain operations."
88+
).mkString("|")
8389
builder += Seq(
8490
"spark.memory.offHeap.size",
91+
AllGlutenConfiguration.configModifiability("spark.memory.offHeap.size"),
8592
"30G",
8693
"The absolute amount of memory which can be used for off-heap allocation, in bytes unless otherwise specified.<br /> Note: Gluten Plugin will leverage this setting to allocate memory space for native usage even offHeap is disabled. <br /> The value is based on your system and it is recommended to set it larger if you are facing Out of Memory issue in Gluten Plugin."
8794
).mkString("|")
8895
builder += Seq(
8996
"spark.shuffle.manager",
97+
AllGlutenConfiguration.configModifiability("spark.shuffle.manager"),
9098
"org.apache.spark.shuffle.sort.ColumnarShuffleManager",
91-
"To turn on Gluten Columnar Shuffle Plugin.").mkString("|")
99+
"To turn on Gluten Columnar Shuffle Plugin."
100+
).mkString("|")
92101
builder += Seq(
93102
"spark.driver.extraClassPath",
103+
AllGlutenConfiguration.configModifiability("spark.driver.extraClassPath"),
94104
"/path/to/gluten_jar_file",
95-
"Gluten Plugin jar file to prepend to the classpath of the driver.").mkString("|")
105+
"Gluten Plugin jar file to prepend to the classpath of the driver."
106+
).mkString("|")
96107
builder += Seq(
97108
"spark.executor.extraClassPath",
109+
AllGlutenConfiguration.configModifiability("spark.executor.extraClassPath"),
98110
"/path/to/gluten_jar_file",
99-
"Gluten Plugin jar file to prepend to the classpath of executors.").mkString("|")
111+
"Gluten Plugin jar file to prepend to the classpath of executors."
112+
).mkString("|")
100113
// scalastyle:on
101114

102115
builder ++=
103116
s"""
104117
|## Gluten configurations
105118
|
106-
| Key | Default | Description
107-
| --- | --- | ---
119+
| Key | Modifiability | Default | Description
120+
| --- | --- | --- | ---
108121
|"""
109122

110123
val allEntries = GlutenConfig.allEntries ++ GlutenCoreConfig.allEntries
@@ -116,16 +129,20 @@ class AllGlutenConfiguration extends AnyFunSuite {
116129
.foreach {
117130
entry =>
118131
val dft = entry.defaultValueString.replace("<", "&lt;").replace(">", "&gt;")
119-
builder += Seq(s"${entry.key}", s"$dft", s"${entry.doc}")
132+
builder += Seq(
133+
s"${entry.key}",
134+
AllGlutenConfiguration.configModifiability(entry.key),
135+
s"$dft",
136+
s"${entry.doc}")
120137
.mkString("|")
121138
}
122139

123140
builder ++=
124141
s"""
125142
|## Gluten *experimental* configurations
126143
|
127-
| Key | Default | Description
128-
| --- | --- | ---
144+
| Key | Modifiability | Default | Description
145+
| --- | --- | --- | ---
129146
|"""
130147

131148
allEntries
@@ -135,7 +152,11 @@ class AllGlutenConfiguration extends AnyFunSuite {
135152
.foreach {
136153
entry =>
137154
val dft = entry.defaultValueString.replace("<", "&lt;").replace(">", "&gt;")
138-
builder += Seq(s"${entry.key}", s"$dft", s"${entry.doc}")
155+
builder += Seq(
156+
s"${entry.key}",
157+
AllGlutenConfiguration.configModifiability(entry.key),
158+
s"$dft",
159+
s"${entry.doc}")
139160
.mkString("|")
140161
}
141162

@@ -147,6 +168,20 @@ class AllGlutenConfiguration extends AnyFunSuite {
147168
}
148169

149170
object AllGlutenConfiguration {
171+
private val Anchor = Character.toString(0x2693)
172+
private val CounterclockwiseArrows = new String(Character.toChars(0x1f504))
173+
174+
val staticConfigLabel: String = s"$Anchor Static"
175+
val dynamicConfigLabel: String = s"$CounterclockwiseArrows Dynamic"
176+
177+
def configModifiability(key: String): String = {
178+
if (SQLConf.get.isModifiable(key)) {
179+
dynamicConfigLabel
180+
} else {
181+
staticConfigLabel
182+
}
183+
}
184+
150185
def isRegenerateGoldenFiles: Boolean = sys.env.get("GLUTEN_UPDATE").contains("1")
151186

152187
def getCodeSourceLocation[T](clazz: Class[T]): String = {

‎gluten-ut/spark40/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -724,6 +724,7 @@ class VeloxTestSettings extends BackendTestSettings {
724724
// `/tmp/test-resource*.py: Permission denied` and can crash JVM.
725725
.exclude("SPARK-33934: Add SparkFile's root dir to env property PATH")
726726
enableSuite[GlutenSparkSqlParserSuite]
727+
.exclude("Checks if SET/RESET can parse all the configurations")
727728
enableSuite[GlutenUnsafeFixedWidthAggregationMapSuite]
728729
enableSuite[GlutenUnsafeKVExternalSorterSuite]
729730
enableSuite[GlutenUnsafeRowSerializerSuite]

‎gluten-ut/spark40/src/test/scala/org/apache/spark/sql/execution/GlutenSparkSqlParserSuite.scala‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,17 @@ package org.apache.spark.sql.execution
1818

1919
import org.apache.spark.sql.GlutenSQLTestsBaseTrait
2020

21-
class GlutenSparkSqlParserSuite extends SparkSqlParserSuite with GlutenSQLTestsBaseTrait {}
21+
class GlutenSparkSqlParserSuite extends SparkSqlParserSuite with GlutenSQLTestsBaseTrait {
22+
testGluten("Checks if SET/RESET can parse all the configurations") {
23+
sqlConf.getAllDefinedConfs.map(_._1).foreach {
24+
key: String =>
25+
val quotedKey = quoteConfigKey(key)
26+
spark.sessionState.sqlParser.parsePlan(s"SET $quotedKey")
27+
spark.sessionState.sqlParser.parsePlan(s"RESET $quotedKey")
28+
}
29+
}
30+
31+
private def quoteConfigKey(key: String): String = {
32+
s"`${key.replace("`", "``")}`"
33+
}
34+
}

‎gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -707,6 +707,7 @@ class VeloxTestSettings extends BackendTestSettings {
707707
// `/tmp/test-resource*.py: Permission denied` and can crash JVM.
708708
.exclude("SPARK-33934: Add SparkFile's root dir to env property PATH")
709709
enableSuite[GlutenSparkSqlParserSuite]
710+
.exclude("Checks if SET/RESET can parse all the configurations")
710711
enableSuite[GlutenUnsafeFixedWidthAggregationMapSuite]
711712
enableSuite[GlutenUnsafeKVExternalSorterSuite]
712713
enableSuite[GlutenUnsafeRowSerializerSuite]

0 commit comments

Comments
 (0)