[GLUTEN-12698][CORE] Add Iceberg read/write offload switches - #12699
Conversation
Gluten had no Iceberg-specific configuration entry. Disabling Iceberg offload meant reaching for a much broader switch: `spark.gluten.sql.columnar.batchscan` for reads, which also falls back Paimon, Hudi and every other batch scan, and `spark.gluten.sql.enable.enhancedFeatures` for writes, a bundled flag documented as covering "iceberg native write and other features". Add 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 live in `gluten-iceberg` rather than a backend module because `OffloadIcebergScan` is shared by the Velox and ClickHouse components, and a backend that later gains Iceberg write support should honour the same write key instead of adding its own. Both are checked inside the offload rules 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. Fixes apache#12698
|
Run Gluten Clickhouse CI on x86 |
|
cc @jinchengchenghh PTAL |
infvg
left a comment
infvg
left a comment
There was a problem hiding this comment.
Generally looks good, just a few nits
Drop the `OffloadIcebergScanBase` / `OffloadIcebergWriteBase` traits and inline the config check into each offload rule, following the existing `OffloadDeltaCommand` precedent. The traits existed only as an injection point for the unit suites. Remove `OffloadIcebergScanSuite`, `OffloadIcebergWriteSuite` and `GlutenIcebergConfigSuite`. The switches are already covered end to end by the integration tests added to `IcebergSuite` and `VeloxIcebergSuite` in this PR.
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
gluten-iceberg/src/test/scala/org/apache/gluten/execution/IcebergSuite.scala:762
- This test checks only that
IcebergScanTransformeris absent, but that still allows otherBatchScanExec-based Gluten transformers to run. To validate the switch truly forces a Spark fallback for Iceberg, it helps to assert the executed plan contains noBatchScanExecTransformerBasefor the Iceberg query.
val icebergDf = spark.sql("select * from iceberg_read_switch_tb")
assert(
!getExecutedPlan(icebergDf).exists(_.isInstanceOf[IcebergScanTransformer]),
"Iceberg scan should fall back")
gluten-iceberg/src/test/scala/org/apache/gluten/execution/IcebergSuite.scala:736
checkSparkPlan[BatchScanExec]is not sufficient to prove the query actually fell back to vanilla Spark, because Gluten batch-scan transformers (including IcebergScanTransformer) are alsoBatchScanExecsubclasses. This can make the test pass even if the scan is still being offloaded via another BatchScanExec-based transformer. Consider asserting that noBatchScanExecTransformerBaseappears in the executed plan (and optionally that a vanillaBatchScanExecdoes).
This issue also appears on line 759 of the same file.
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")
|
Run Gluten Clickhouse CI on x86 |
|
cc @infvg @jinchengchenghh PTAL |
|
@jinchengchenghh Any more questions? |
|
@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? Line 46 in 6273a48 |
|
I did notice that we still have several plan-level configurations, for example:
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. |
|
@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 |
The |
What changes are proposed in this pull request?
Gluten has no Iceberg-specific config today. Disabling Iceberg offload means using a much broader
switch:
spark.gluten.sql.columnar.batchscanfor reads (also falls back Paimon/Hudi and all otherbatch scans), and
spark.gluten.sql.enable.enhancedFeaturesfor writes (a bundled flag).Add two backend-agnostic switches, both defaulting to
trueso behaviour is unchanged:They live in
gluten-icebergrather than a backend module, sinceOffloadIcebergScanis shared bythe Velox and ClickHouse components and a backend later gaining Iceberg write should reuse the same
key. Both are checked inside the offload rules rather than at rule-injection time, so they stay
modifiable per session. The write switch is AND-ed with
enhancedFeaturesrather than replacing it.How was this patch tested?
Newly added tests.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-opus-5
Related issue: #12698