Copilot commented on code in PR #12914:
URL: https://github.com/apache/gluten/pull/12914#discussion_r3873655842
##########
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 PR description focuses on adding Iceberg runtime to the UT test
classpath, but this diff also changes production behavior by removing
component-driven Spark SQL extension aggregation and only appending Gluten’s
extension. Please either (a) update the PR title/description to explicitly call
out this behavior/API change and rationale, or (b) split the extension-wiring
refactor into a separate PR to keep this change set scoped to the
dependency/classpath goal.
##########
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:
This new extension-wiring logic no longer has UT coverage after
`GlutenSessionExtensionsSuite` was removed. Please add/restore a UT that
asserts: (1) Gluten extension is appended when absent, (2) it isn’t duplicated
when already present, and (3) preconfigured extensions (e.g., Iceberg) are
preserved and order remains stable.
##########
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")
Review Comment:
Setting `spark.sql.extensions` directly to a single class overwrites any
extensions already configured in `super.sparkConf`. Prefer merging/appending
(comma-separated) so this suite doesn’t accidentally disable other extensions
that may be required by the test harness.
##########
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 `Component` is an API break that
eliminates a general mechanism for components to declare required Spark SQL
extensions. If the goal is specifically to avoid auto-enabling Iceberg
extensions in UT/CI, consider keeping the method (possibly with a default
`Nil`) and changing only the Iceberg component (or gating behavior behind
config), rather than removing the API entirely.
##########
backends-velox/src-iceberg/main/scala/org/apache/gluten/component/VeloxIcebergComponent.scala:
##########
@@ -23,19 +23,15 @@ import org.apache.gluten.extension.injector.Injector
import org.apache.spark.util.SparkReflectionUtil
class VeloxIcebergComponent extends Component {
- private val icebergSparkSessionExtension =
- "org.apache.iceberg.spark.extensions.IcebergSparkSessionExtensions"
-
override def name(): String = "velox-iceberg"
override def dependencies(): Seq[Class[_ <: Component]] =
classOf[VeloxBackend] :: Nil
override def isRuntimeCompatible: Boolean = {
- SparkReflectionUtil.isClassPresent(icebergSparkSessionExtension)
+ SparkReflectionUtil.isClassPresent(
+ "org.apache.iceberg.spark.extensions.IcebergSparkSessionExtensions")
Review Comment:
The Iceberg extension FQCN is now duplicated across modules (also set as a
literal in `IcebergSuite`). Consider centralizing it as a shared constant
(e.g., in an Iceberg utility/object) so refactors don’t silently diverge and to
reduce typo risk.
##########
gluten-ut/pom.xml:
##########
@@ -200,6 +200,20 @@
</build>
<profiles>
+ <profile>
+ <id>iceberg</id>
+ <activation>
+ <activeByDefault>false</activeByDefault>
+ </activation>
Review Comment:
`<activeByDefault>false</activeByDefault>` is Maven’s default behavior for
profiles. You can simplify by removing the `<activation>` block unless there’s
a specific reason to keep it explicit.
--
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]
