Copilot commented on code in PR #12913:
URL: https://github.com/apache/gluten/pull/12913#discussion_r3871326876
##########
gluten-iceberg/src/test/scala/org/apache/gluten/execution/IcebergSuite.scala:
##########
@@ -37,21 +37,14 @@ abstract class IcebergSuite extends
WholeStageTransformerSuite {
.set("spark.memory.offHeap.size", "2g")
.set("spark.unsafe.exceptionOnMemoryLeak", "true")
.set("spark.sql.autoBroadcastJoinThreshold", "-1")
+ .set(
+ "spark.sql.extensions",
+ "org.apache.iceberg.spark.extensions.IcebergSparkSessionExtensions")
.set("spark.sql.catalog.spark_catalog",
"org.apache.iceberg.spark.SparkCatalog")
.set("spark.sql.catalog.spark_catalog.type", "hadoop")
.set("spark.sql.catalog.spark_catalog.warehouse",
s"file://$rootPath/tpch-data-iceberg-velox")
Review Comment:
Using the raw string key `\"spark.sql.extensions\"` is more error-prone than
referencing Spark’s config entry constant (e.g.,
`StaticSQLConf.SPARK_SESSION_EXTENSIONS.key`) used elsewhere in the project.
Consider switching to the constant to prevent typos and keep config usage
consistent.
##########
gluten-core/src/main/scala/org/apache/gluten/GlutenPlugin.scala:
##########
@@ -50,11 +50,17 @@ private[gluten] class GlutenDriverPlugin extends
DriverPlugin with Logging {
override def init(sc: SparkContext, pluginContext: PluginContext):
util.Map[String, String] = {
val conf = pluginContext.conf()
- val components = Component.sorted()
- configureSessionExtensions(conf, components)
+ // Spark SQL extensions
+ val extensionSeq = conf.get(SPARK_SESSION_EXTENSIONS).getOrElse(Seq.empty)
+ if
(!extensionSeq.toSet.contains(GlutenSessionExtensions.GLUTEN_SESSION_EXTENSION_NAME))
{
+ conf.set(
+ SPARK_SESSION_EXTENSIONS,
+ extensionSeq :+ GlutenSessionExtensions.GLUTEN_SESSION_EXTENSION_NAME)
+ }
Review Comment:
Building a Set just to test membership (`extensionSeq.toSet.contains(...)`)
allocates unnecessarily. Prefer `extensionSeq.contains(...)` to avoid the extra
allocation and improve startup cost (especially since this runs during driver
plugin init).
##########
gluten-core/src/main/scala/org/apache/gluten/component/Component.scala:
##########
@@ -79,8 +79,6 @@ trait Component {
def info(): Map[String, String] = Map.empty
def dependencies(): Seq[Class[_ <: Component]]
- def sparkSessionExtensions(): Seq[String] = Nil
-
/** Spark listeners. */
def onDriverStart(sc: SparkContext, pc: PluginContext): Unit = {}
def onDriverShutdown(): Unit = {}
Review Comment:
Removing `sparkSessionExtensions()` from the public `Component` trait is a
source/binary incompatible API change for any external/3rd-party components. If
this trait is part of the supported extension API, consider keeping the method
(e.g., deprecated with a migration note) to avoid breaking downstream
implementations, even if Gluten no longer consumes it.
##########
gluten-iceberg/src/test/scala/org/apache/gluten/execution/IcebergSuite.scala:
##########
@@ -37,21 +37,14 @@ abstract class IcebergSuite extends
WholeStageTransformerSuite {
.set("spark.memory.offHeap.size", "2g")
.set("spark.unsafe.exceptionOnMemoryLeak", "true")
.set("spark.sql.autoBroadcastJoinThreshold", "-1")
+ .set(
+ "spark.sql.extensions",
+ "org.apache.iceberg.spark.extensions.IcebergSparkSessionExtensions")
.set("spark.sql.catalog.spark_catalog",
"org.apache.iceberg.spark.SparkCatalog")
.set("spark.sql.catalog.spark_catalog.type", "hadoop")
.set("spark.sql.catalog.spark_catalog.warehouse",
s"file://$rootPath/tpch-data-iceberg-velox")
}
Review Comment:
This suite previously validated that Iceberg system procedures were
registered (via the plugin). That coverage is removed, but the behavior change
is subtle and easy to regress. Consider adding a replacement assertion that
procedures/parsing are available when `spark.sql.extensions` is explicitly set
(new intended behavior), so the test suite still guards Iceberg SQL integration.
##########
gluten-core/src/main/scala/org/apache/gluten/GlutenPlugin.scala:
##########
@@ -50,11 +50,17 @@ private[gluten] class GlutenDriverPlugin extends
DriverPlugin with Logging {
override def init(sc: SparkContext, pluginContext: PluginContext):
util.Map[String, String] = {
val conf = pluginContext.conf()
- val components = Component.sorted()
- configureSessionExtensions(conf, components)
+ // Spark SQL extensions
+ val extensionSeq = conf.get(SPARK_SESSION_EXTENSIONS).getOrElse(Seq.empty)
+ if
(!extensionSeq.toSet.contains(GlutenSessionExtensions.GLUTEN_SESSION_EXTENSION_NAME))
{
+ conf.set(
+ SPARK_SESSION_EXTENSIONS,
+ extensionSeq :+ GlutenSessionExtensions.GLUTEN_SESSION_EXTENSION_NAME)
+ }
Review Comment:
The deleted `GlutenSessionExtensionsSuite` previously ensured session
extensions were appended exactly once and preserved configured extensions. With
the new direct-append logic, it would be good to reintroduce equivalent
coverage (e.g., a unit test asserting (1) configured extensions remain, (2)
Gluten extension is appended once, and (3) repeated init/configure is
idempotent).
--
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]
