Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
jackylee-ch merged PR #12699: URL: https://github.com/apache/gluten/pull/12699 -- 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]
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
jackylee-ch commented on PR #12699: URL: https://github.com/apache/gluten/pull/12699#issuecomment-5238970550 > @jackylee-ch could you please clarify whether there is a defined priority between these two sets of configurations? The `enableNativeWrite` only takes effect when plan-level configs are true, but they're ANDed — no priority. -- 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]
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
zhouyuan commented on PR #12699: URL: https://github.com/apache/gluten/pull/12699#issuecomment-5238592023 @jackylee-ch could you please clarify whether there is a defined priority between these two sets of configurations? We may need to define a similar configuration for Delta Cc: @zhztheplayer @malinjawi -- 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]
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
jackylee-ch commented on PR #12699: URL: https://github.com/apache/gluten/pull/12699#issuecomment-5238205956 I did notice that we still have several plan-level configurations, for example: * `spark.gluten.sql.columnar.appendData` * `spark.gluten.sql.columnar.replaceData` * `spark.gluten.sql.columnar.overwriteByExpression` * `spark.gluten.sql.columnar.overwritePartitionsDynamic` * `spark.gluten.sql.columnar.writeToDataSourceV2` My understanding is that these options are mainly used to control fallback for specific plans or write paths, rather than enabling or disabling a generic capability. Considering that Gluten may support more table/file formats in the future, I think these plan-level switches can still be useful as fine-grained fallback controls, especially when support varies across formats or backends. Therefore, I prefer to keep them as-is for now rather than consolidating or removing them as part of this change. -- 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]
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
zhouyuan commented on PR #12699: URL: https://github.com/apache/gluten/pull/12699#issuecomment-5238025345 @jackylee-ch thanks for the patch, initially we have a config to control the write behavior, could you please help to remove this as well? `spark.gluten.sql.columnar.appendData ` https://github.com/apache/gluten/blob/6273a48a8016c13a33e1c254a140874e8de2717c/docs/Configuration.md?plain=1#L46 -- 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]
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
jackylee-ch commented on PR #12699: URL: https://github.com/apache/gluten/pull/12699#issuecomment-5235205305 @jinchengchenghh Any more questions? also cc @zhouyuan -- 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]
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
jackylee-ch commented on PR #12699: URL: https://github.com/apache/gluten/pull/12699#issuecomment-5202761396 cc @infvg @jinchengchenghh PTAL -- 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]
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
jackylee-ch commented on code in PR #12699:
URL: https://github.com/apache/gluten/pull/12699#discussion_r3725472355
##
backends-velox/src-iceberg/main/scala/org/apache/gluten/extension/OffloadIcebergWrite.scala:
##
@@ -28,43 +28,71 @@ import org.apache.spark.sql.execution.datasources.v2._
import org.apache.iceberg.spark.source.IcebergWriteUtil.supportsWrite
+// The write switch is checked inside each rule below rather than at rule
injection time so that
Review Comment:
Removed.
##
gluten-iceberg/src/main/scala/org/apache/gluten/config/GlutenIcebergConfig.scala:
##
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.config
+
+import org.apache.spark.sql.internal.SQLConf
+
+class GlutenIcebergConfig(conf: SQLConf) extends GlutenCoreConfig(conf) {
+ import GlutenIcebergConfig._
+
+ def enableNativeRead: Boolean = getConf(ENABLE_NATIVE_READ)
+
+ def enableNativeWrite: Boolean = getConf(ENABLE_NATIVE_WRITE)
+}
+
+/**
+ * Configurations of the Iceberg component. They are backend-agnostic on
purpose: the read path
+ * ([[org.apache.gluten.extension.OffloadIcebergScan]]) is shared by all
backends, and a backend
+ * newly supporting Iceberg write is expected to honor the same write switch
rather than introduce
+ * its own key.
+ */
Review Comment:
Removed.
##
gluten-iceberg/src/main/scala/org/apache/gluten/extension/OffloadIcebergScan.scala:
##
@@ -27,10 +27,17 @@ import org.apache.spark.sql.execution.SparkPlan
import org.apache.spark.sql.execution.datasources.v2.BatchScanExec
case class OffloadIcebergScan() extends OffloadSingleNode {
- override def offload(plan: SparkPlan): SparkPlan = plan match {
-case scan: BatchScanExec if
IcebergScanTransformer.supportsBatchScan(scan.scan) =>
- IcebergScanTransformer(scan)
-case other => other
+ override def offload(plan: SparkPlan): SparkPlan = {
+// The switch is checked here rather than at rule injection time so that
it stays
+// modifiable at runtime.
Review Comment:
Removed.
##
gluten-iceberg/src/test/scala/org/apache/gluten/execution/IcebergSuite.scala:
##
@@ -717,4 +720,54 @@ abstract class IcebergSuite extends
WholeStageTransformerSuite {
e.getCause != null && e.getCause.getMessage.contains("null"))
}
}
+
+ test("iceberg scan falls back when native read is disabled") {
+withTable("iceberg_read_switch_tb") {
+ spark.sql("""
+ |create table iceberg_read_switch_tb using iceberg as
+ |(select 1 as col1, 2 as col2)
+ |""".stripMargin)
+
+ withSQLConf(GlutenIcebergConfig.ENABLE_NATIVE_READ.key -> "false") {
+val df = spark.sql("select * from iceberg_read_switch_tb")
+checkSparkPlan[BatchScanExec](df)
+assert(
+ !getExecutedPlan(df).exists(_.isInstanceOf[IcebergScanTransformer]),
+ "Iceberg scan should not be offloaded when native read is disabled")
+checkAnswer(df, Seq(Row(1, 2)))
+ }
+
+ // The switch is dynamic: offload resumes once it is back to the default.
+ runQueryAndCompare("select * from iceberg_read_switch_tb") {
+checkGlutenPlan[IcebergScanTransformer]
+ }
+}
+ }
+
+ test("disabling iceberg native read keeps other scans offloaded") {
+withTable("iceberg_read_switch_tb") {
+ spark.sql("""
+ |create table iceberg_read_switch_tb using iceberg as
+ |(select 1 as col1)
+ |""".stripMargin)
+
+ withTempPath {
+path =>
+ spark.range(5).toDF("col1").write.parquet(path.getCanonicalPath)
+
+ withSQLConf(GlutenIcebergConfig.ENABLE_NATIVE_READ.key -> "false") {
+val icebergDf = spark.sql("select * from iceberg_read_switch_tb")
+assert(
+
!getExecutedPlan(icebergDf).exists(_.isInstanceOf[IcebergScanTransformer]),
+ "Iceberg scan should fall back")
+
+// The switch is scoped to Iceberg: scans of other formats keep
being offloaded, which
+// is the whole point of not reusing
spark.gluten.sql.columnar.batchscan for this.
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
github-actions[bot] commented on PR #12699: URL: https://github.com/apache/gluten/pull/12699#issuecomment-5199689014 Run Gluten Clickhouse CI on x86 -- 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]
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
infvg commented on code in PR #12699:
URL: https://github.com/apache/gluten/pull/12699#discussion_r3724337520
##
gluten-iceberg/src/main/scala/org/apache/gluten/config/GlutenIcebergConfig.scala:
##
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.config
+
+import org.apache.spark.sql.internal.SQLConf
+
+class GlutenIcebergConfig(conf: SQLConf) extends GlutenCoreConfig(conf) {
+ import GlutenIcebergConfig._
+
+ def enableNativeRead: Boolean = getConf(ENABLE_NATIVE_READ)
+
+ def enableNativeWrite: Boolean = getConf(ENABLE_NATIVE_WRITE)
+}
+
+/**
+ * Configurations of the Iceberg component. They are backend-agnostic on
purpose: the read path
+ * ([[org.apache.gluten.extension.OffloadIcebergScan]]) is shared by all
backends, and a backend
+ * newly supporting Iceberg write is expected to honor the same write switch
rather than introduce
+ * its own key.
+ */
Review Comment:
This comment can be removed; it's talking about the configs that are being
added
##
backends-velox/src-iceberg/main/scala/org/apache/gluten/extension/OffloadIcebergWrite.scala:
##
@@ -28,43 +28,71 @@ import org.apache.spark.sql.execution.datasources.v2._
import org.apache.iceberg.spark.source.IcebergWriteUtil.supportsWrite
+// The write switch is checked inside each rule below rather than at rule
injection time so that
Review Comment:
This comment can be removed
##
gluten-iceberg/src/test/scala/org/apache/gluten/execution/IcebergSuite.scala:
##
@@ -717,4 +720,54 @@ abstract class IcebergSuite extends
WholeStageTransformerSuite {
e.getCause != null && e.getCause.getMessage.contains("null"))
}
}
+
+ test("iceberg scan falls back when native read is disabled") {
+withTable("iceberg_read_switch_tb") {
+ spark.sql("""
+ |create table iceberg_read_switch_tb using iceberg as
+ |(select 1 as col1, 2 as col2)
+ |""".stripMargin)
+
+ withSQLConf(GlutenIcebergConfig.ENABLE_NATIVE_READ.key -> "false") {
+val df = spark.sql("select * from iceberg_read_switch_tb")
+checkSparkPlan[BatchScanExec](df)
+assert(
+ !getExecutedPlan(df).exists(_.isInstanceOf[IcebergScanTransformer]),
+ "Iceberg scan should not be offloaded when native read is disabled")
+checkAnswer(df, Seq(Row(1, 2)))
+ }
+
+ // The switch is dynamic: offload resumes once it is back to the default.
+ runQueryAndCompare("select * from iceberg_read_switch_tb") {
+checkGlutenPlan[IcebergScanTransformer]
+ }
+}
+ }
+
+ test("disabling iceberg native read keeps other scans offloaded") {
+withTable("iceberg_read_switch_tb") {
+ spark.sql("""
+ |create table iceberg_read_switch_tb using iceberg as
+ |(select 1 as col1)
+ |""".stripMargin)
+
+ withTempPath {
+path =>
+ spark.range(5).toDF("col1").write.parquet(path.getCanonicalPath)
+
+ withSQLConf(GlutenIcebergConfig.ENABLE_NATIVE_READ.key -> "false") {
+val icebergDf = spark.sql("select * from iceberg_read_switch_tb")
+assert(
+
!getExecutedPlan(icebergDf).exists(_.isInstanceOf[IcebergScanTransformer]),
+ "Iceberg scan should fall back")
+
+// The switch is scoped to Iceberg: scans of other formats keep
being offloaded, which
+// is the whole point of not reusing
spark.gluten.sql.columnar.batchscan for this.
Review Comment:
We can remove this too
##
gluten-iceberg/src/main/scala/org/apache/gluten/extension/OffloadIcebergScan.scala:
##
@@ -27,10 +27,17 @@ import org.apache.spark.sql.execution.SparkPlan
import org.apache.spark.sql.execution.datasources.v2.BatchScanExec
case class OffloadIcebergScan() extends OffloadSingleNode {
- override def offload(plan: SparkPlan): SparkPlan = plan match {
-case scan: BatchScanExec if
IcebergScanTransformer.supportsBatchScan(scan.scan) =>
- IcebergScanTransformer(scan)
-case other => other
+ override def offload(plan: SparkPlan): SparkPlan = {
+// The switch is
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
infvg commented on code in PR #12699: URL: https://github.com/apache/gluten/pull/12699#discussion_r3724327828 ## backends-velox/src-iceberg/main/scala/org/apache/gluten/extension/OffloadIcebergWrite.scala: ## @@ -28,43 +28,71 @@ import org.apache.spark.sql.execution.datasources.v2._ import org.apache.iceberg.spark.source.IcebergWriteUtil.supportsWrite +// The write switch is checked inside each rule below rather than at rule injection time so that Review Comment: This comment can be removed ## backends-velox/src-iceberg/main/scala/org/apache/gluten/extension/OffloadIcebergWrite.scala: ## @@ -28,43 +28,71 @@ import org.apache.spark.sql.execution.datasources.v2._ import org.apache.iceberg.spark.source.IcebergWriteUtil.supportsWrite +// The write switch is checked inside each rule below rather than at rule injection time so that Review Comment: This comment can be removed -- 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]
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
github-actions[bot] commented on PR #12699: URL: https://github.com/apache/gluten/pull/12699#issuecomment-5195472041 Run Gluten Clickhouse CI on x86 -- 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]
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
jackylee-ch commented on code in PR #12699:
URL: https://github.com/apache/gluten/pull/12699#discussion_r3722880509
##
gluten-iceberg/src/test/scala/org/apache/gluten/extension/OffloadIcebergScanSuite.scala:
##
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.extension
+
+import org.apache.gluten.config.GlutenIcebergConfig
+
+import org.apache.spark.sql.catalyst.expressions.Attribute
+import org.apache.spark.sql.execution.{LeafExecNode, SparkPlan}
+import org.apache.spark.sql.internal.SQLConf
+
+import org.scalatest.funsuite.AnyFunSuite
+
+class OffloadIcebergScanSuite extends AnyFunSuite {
+
+ private case class DummyPlan() extends LeafExecNode {
+override def output: Seq[Attribute] = Seq.empty
+override protected def doExecute() = throw new
UnsupportedOperationException()
+ }
+
+ private case class OffloadedPlan() extends LeafExecNode {
+override def output: Seq[Attribute] = Seq.empty
+override protected def doExecute() = throw new
UnsupportedOperationException()
+ }
+
+ /** Offloads unconditionally, so the only thing that can stop it is the
config gate. */
+ private case class AlwaysOffload() extends OffloadIcebergScanBase {
+override protected def offloadScan(plan: SparkPlan): SparkPlan =
OffloadedPlan()
+ }
+
+ private def withNativeRead[T](enabled: Boolean)(body: => T): T = {
+val conf = SQLConf.get
+val key = GlutenIcebergConfig.ENABLE_NATIVE_READ.key
+conf.setConfString(key, enabled.toString)
+try body
+finally conf.unsetConf(key)
+ }
+
+ test("scan offload rule skips offloading when native read is disabled") {
Review Comment:
Removed. Also removed `GlutenIcebergConfigSuite` for the same reason — the
read switch is covered by the two integration tests added to `IcebergSuite`
(fallback + dynamic re-offload, and Iceberg-only scoping while Parquet scans
stay offloaded).
--
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]
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
jackylee-ch commented on code in PR #12699:
URL: https://github.com/apache/gluten/pull/12699#discussion_r3722879053
##
gluten-iceberg/src/main/scala/org/apache/gluten/extension/OffloadIcebergScan.scala:
##
@@ -26,8 +26,23 @@ import org.apache.gluten.extension.injector.Injector
import org.apache.spark.sql.execution.SparkPlan
import org.apache.spark.sql.execution.datasources.v2.BatchScanExec
-case class OffloadIcebergScan() extends OffloadSingleNode {
- override def offload(plan: SparkPlan): SparkPlan = plan match {
+/**
+ * Base of the Iceberg scan offload rule. The switch is checked here rather
than at rule injection
+ * time so that it stays modifiable at runtime.
+ */
+trait OffloadIcebergScanBase extends OffloadSingleNode {
Review Comment:
Removed the trait; the config check is now inlined in
`OffloadIcebergScan.offload`. Did the same on the write side
(`OffloadIcebergWriteBase` is gone, the 5 rules each check the switch
directly), following the `OffloadDeltaCommand` precedent.
##
backends-velox/src-iceberg/test/scala/org/apache/gluten/extension/OffloadIcebergWriteSuite.scala:
##
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.extension
+
+import org.apache.gluten.config.GlutenIcebergConfig
+
+import org.apache.spark.sql.catalyst.expressions.Attribute
+import org.apache.spark.sql.execution.{LeafExecNode, SparkPlan}
+import org.apache.spark.sql.internal.SQLConf
+
+import org.scalatest.funsuite.AnyFunSuite
+
+class OffloadIcebergWriteSuite extends AnyFunSuite {
+
+ private case class DummyPlan() extends LeafExecNode {
+override def output: Seq[Attribute] = Seq.empty
+override protected def doExecute() = throw new
UnsupportedOperationException()
+ }
+
+ private case class OffloadedPlan() extends LeafExecNode {
+override def output: Seq[Attribute] = Seq.empty
+override protected def doExecute() = throw new
UnsupportedOperationException()
+ }
+
+ /** Offloads unconditionally, so the only thing that can stop it is the
config gate. */
+ private case class AlwaysOffload() extends OffloadIcebergWriteBase {
+override protected def offloadWrite(plan: SparkPlan): SparkPlan =
OffloadedPlan()
+ }
+
+ private def withNativeWrite[T](enabled: Boolean)(body: => T): T = {
+val conf = SQLConf.get
+val key = GlutenIcebergConfig.ENABLE_NATIVE_WRITE.key
+conf.setConfString(key, enabled.toString)
+try body
+finally conf.unsetConf(key)
+ }
+
+ test("write offload rules skip offloading when native write is disabled") {
Review Comment:
Removed. The write switch is covered by the integration test added to
`VeloxIcebergSuite` in this PR.
--
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]
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
infvg commented on code in PR #12699:
URL: https://github.com/apache/gluten/pull/12699#discussion_r3721233292
##
backends-velox/src-iceberg/test/scala/org/apache/gluten/extension/OffloadIcebergWriteSuite.scala:
##
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.extension
+
+import org.apache.gluten.config.GlutenIcebergConfig
+
+import org.apache.spark.sql.catalyst.expressions.Attribute
+import org.apache.spark.sql.execution.{LeafExecNode, SparkPlan}
+import org.apache.spark.sql.internal.SQLConf
+
+import org.scalatest.funsuite.AnyFunSuite
+
+class OffloadIcebergWriteSuite extends AnyFunSuite {
+
+ private case class DummyPlan() extends LeafExecNode {
+override def output: Seq[Attribute] = Seq.empty
+override protected def doExecute() = throw new
UnsupportedOperationException()
+ }
+
+ private case class OffloadedPlan() extends LeafExecNode {
+override def output: Seq[Attribute] = Seq.empty
+override protected def doExecute() = throw new
UnsupportedOperationException()
+ }
+
+ /** Offloads unconditionally, so the only thing that can stop it is the
config gate. */
+ private case class AlwaysOffload() extends OffloadIcebergWriteBase {
+override protected def offloadWrite(plan: SparkPlan): SparkPlan =
OffloadedPlan()
+ }
+
+ private def withNativeWrite[T](enabled: Boolean)(body: => T): T = {
+val conf = SQLConf.get
+val key = GlutenIcebergConfig.ENABLE_NATIVE_WRITE.key
+conf.setConfString(key, enabled.toString)
+try body
+finally conf.unsetConf(key)
+ }
+
+ test("write offload rules skip offloading when native write is disabled") {
Review Comment:
There's already integration tests for this in VeloxIcebergSuite, we can
remove this class.
##
gluten-iceberg/src/main/scala/org/apache/gluten/extension/OffloadIcebergScan.scala:
##
@@ -26,8 +26,23 @@ import org.apache.gluten.extension.injector.Injector
import org.apache.spark.sql.execution.SparkPlan
import org.apache.spark.sql.execution.datasources.v2.BatchScanExec
-case class OffloadIcebergScan() extends OffloadSingleNode {
- override def offload(plan: SparkPlan): SparkPlan = plan match {
+/**
+ * Base of the Iceberg scan offload rule. The switch is checked here rather
than at rule injection
+ * time so that it stays modifiable at runtime.
+ */
+trait OffloadIcebergScanBase extends OffloadSingleNode {
Review Comment:
This abstraction isn't necessary IMO, we can just move the logic to
OffloadIcebergScan
##
gluten-iceberg/src/test/scala/org/apache/gluten/extension/OffloadIcebergScanSuite.scala:
##
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.extension
+
+import org.apache.gluten.config.GlutenIcebergConfig
+
+import org.apache.spark.sql.catalyst.expressions.Attribute
+import org.apache.spark.sql.execution.{LeafExecNode, SparkPlan}
+import org.apache.spark.sql.internal.SQLConf
+
+import org.scalatest.funsuite.AnyFunSuite
+
+class OffloadIcebergScanSuite extends AnyFunSuite {
+
+ private case class DummyPlan() extends LeafExecNode {
+override def output: Seq[Attribute] = Seq.empty
+override protected def doExecute() = throw new
UnsupportedOperationException()
+ }
+
+ private case class OffloadedPlan() extends LeafExecNode {
+override def output: Seq[Attribute] = Seq.empty
+override protected def doExecute() = throw new
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
infvg commented on code in PR #12699:
URL: https://github.com/apache/gluten/pull/12699#discussion_r3721099681
##
gluten-iceberg/src/main/scala/org/apache/gluten/config/GlutenIcebergConfig.scala:
##
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.config
+
+import org.apache.spark.sql.internal.SQLConf
+
+class GlutenIcebergConfig(conf: SQLConf) extends GlutenCoreConfig(conf) {
+ import GlutenIcebergConfig._
+
+ def enableNativeRead: Boolean = getConf(ENABLE_NATIVE_READ)
+
+ def enableNativeWrite: Boolean = getConf(ENABLE_NATIVE_WRITE)
+}
+
+/**
+ * Configurations of the Iceberg component. They are backend-agnostic on
purpose: the read path
+ * ([[org.apache.gluten.extension.OffloadIcebergScan]]) is shared by all
backends, and a backend
+ * newly supporting Iceberg write is expected to honor the same write switch
rather than introduce
+ * its own key.
+ */
+object GlutenIcebergConfig extends ConfigRegistry {
+
+ def get: GlutenIcebergConfig = {
+new GlutenIcebergConfig(SQLConf.get)
+ }
+
+ val ENABLE_NATIVE_READ: ConfigEntry[Boolean] =
Review Comment:
We could move these to GlutenConfig as opposed to making a new class
##
gluten-iceberg/src/main/scala/org/apache/gluten/config/GlutenIcebergConfig.scala:
##
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.config
+
+import org.apache.spark.sql.internal.SQLConf
+
+class GlutenIcebergConfig(conf: SQLConf) extends GlutenCoreConfig(conf) {
+ import GlutenIcebergConfig._
+
+ def enableNativeRead: Boolean = getConf(ENABLE_NATIVE_READ)
+
+ def enableNativeWrite: Boolean = getConf(ENABLE_NATIVE_WRITE)
+}
+
+/**
+ * Configurations of the Iceberg component. They are backend-agnostic on
purpose: the read path
+ * ([[org.apache.gluten.extension.OffloadIcebergScan]]) is shared by all
backends, and a backend
+ * newly supporting Iceberg write is expected to honor the same write switch
rather than introduce
+ * its own key.
+ */
+object GlutenIcebergConfig extends ConfigRegistry {
+
+ def get: GlutenIcebergConfig = {
+new GlutenIcebergConfig(SQLConf.get)
+ }
+
+ val ENABLE_NATIVE_READ: ConfigEntry[Boolean] =
Review Comment:
We could move these to GlutenConfig as opposed to making a new class
--
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]
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
jackylee-ch commented on PR #12699: URL: https://github.com/apache/gluten/pull/12699#issuecomment-5192373604 cc @jinchengchenghh PTAL -- 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]
Re: [PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
github-actions[bot] commented on PR #12699: URL: https://github.com/apache/gluten/pull/12699#issuecomment-5189784155 Run Gluten Clickhouse CI on x86 -- 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]
[PR] [GLUTEN-12698][CORE] Add Iceberg read/write offload switches [gluten]
jackylee-ch opened a new pull request, #12699: URL: https://github.com/apache/gluten/pull/12699 ## What changes are proposed in this pull request? Gluten has no Iceberg-specific configuration entry today, so disabling Iceberg offload means reaching for a much broader switch: | Path | Only way to disable before this PR | Problem | |---|---|---| | Read | `spark.gluten.sql.columnar.batchscan` | Generic DSv2 switch — also falls back Paimon, Hudi and every other batch scan. | | Write | `spark.gluten.sql.enable.enhancedFeatures` | Bundled flag, documented as "iceberg native write **and other features**". | This PR adds two backend-agnostic switches, both defaulting to `true` so behaviour is unchanged: ``` spark.gluten.sql.columnar.iceberg.enableNativeRead spark.gluten.sql.columnar.iceberg.enableNativeWrite ``` They are declared in a new `GlutenIcebergConfig` in `gluten-iceberg` rather than in a backend module, because `OffloadIcebergScan` is shared by `VeloxIcebergComponent` and `CHIcebergComponent`, and a backend that later gains Iceberg write support should honour the same write key instead of introducing its own. Both switches are checked inside the offload rules (`OffloadIcebergScanBase` / `OffloadIcebergWriteBase`) rather than at rule-injection time, so they stay modifiable per session. The write switch is AND-ed with the existing `enhancedFeatures` gate rather than replacing it. Also documents both keys in `docs/get-started/VeloxIceberg.md`. Note they do not appear in `docs/velox-configuration.md`, because `AllVeloxConfiguration` only walks `VeloxConfig.allEntries` — the same reason `VeloxDeltaConfig`'s key is absent there today. Extending that generator to cover component-level configs is left as a follow-up. ## How was this patch tested? New unit suites covering the gate itself, verified by inverting the condition and watching both assertions fail before restoring it: - `gluten-iceberg`: `GlutenIcebergConfigSuite` (key names, defaults, runtime modifiability, reading from the active `SQLConf`), `OffloadIcebergScanSuite` (gate blocks offload, gate is consulted per call rather than captured at rule construction). - `backends-velox`: `OffloadIcebergWriteSuite` (same for the write gate). New end-to-end tests: - `IcebergSuite` — Iceberg scan falls back to `BatchScanExec` with the read switch off, offload resumes when it is restored, and a Parquet scan in the same session stays offloaded (`FileSourceScanExecTransformer`), which is the behaviour the generic batch-scan switch cannot express. - `VeloxIcebergSuite` (enhanced) — Iceberg write falls back to vanilla `AppendDataExec` with the write switch off while reads stay offloaded, and offload resumes when it is restored. Ran locally on macOS with `-Pspark-3.5 -Pscala-2.12 -Pbackends-velox -Piceberg`: the three unit suites pass (7 tests). `spotless:check`, `scalastyle:check` and `checkstyle:check` pass for both modules, and `./dev/format-scala-code.sh` is applied. The two end-to-end suites need a native build and are left to CI. ## Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude claude-opus-4-8 -- 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]
