Spark: Add session-level split size override#16154
Conversation
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
… configurations - Removed the session configuration for split size from SparkReadConf and SparkSQLProperties. - Updated SparkReadConf documentation to clarify the precedence of table-scoped session configurations over global settings. - Added tests to verify that table-scoped session configurations take precedence over global configurations and that options take precedence over table-scoped configurations. Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
…ation for scan planning
# Conflicts: # spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkConfParser.java # spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkReadConf.java # spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkSQLProperties.java
| return confParser | ||
| .longConf() | ||
| .option(SparkReadOptions.SPLIT_SIZE) | ||
| .sessionConf(SparkSQLProperties.SPLIT_SIZE) |
There was a problem hiding this comment.
splitSizeOption() is the gate term in SparkScan.java:370 (splitSizeOption() == null && adaptiveSplitSizeEnabled()), so adding SPLIT_SIZE here makes setting the session conf disable adaptive split sizing, which defaults to on. The table-property path leaves splitSizeOption() null and keeps adaptive sizing enabled, so the same split-size value behaves differently depending on whether it comes from this session conf or read.split.target-size. If that is intended (honor an explicit session split size exactly, as the read option does), document it in the precedence javadoc and add a test for the interaction; otherwise the session conf should not suppress adaptive sizing.
| public static final String ADVISORY_PARTITION_SIZE = "spark.sql.iceberg.advisory-partition-size"; | ||
|
|
||
| // Overrides the split target size for scan planning | ||
| public static final String SPLIT_SIZE = "spark.sql.iceberg.split-size"; |
There was a problem hiding this comment.
This adds a user-facing session conf but nothing in docs/docs/spark-configuration.md, where every other spark.sql.iceberg.* conf is listed (the adaptive-split confs from #16088 are at lines 210-211). Add spark.sql.iceberg.split-size, and the table-scoped spark.sql.iceberg.split-size.<table-name> form, to that table.
|
Context for reviewers - this capability has prior history worth reconciling before a deep review. The same feature was proposed twice before and closed without merging (#10336 and its revival #13677). On #10336, @szehon-ho objected to a bare global This PR's table-scoped key ( There is also a competing open PR #16185 proposing the same capability with different naming ( Two related notes for whoever picks this up:
|
| } | ||
|
|
||
| String sparkTableSessionConfValue = | ||
| sessionConf.get(toCamelCase(tableSessionConfName), null); |
There was a problem hiding this comment.
toCamelCase is applied to the full composed key including the table.name() suffix, so any hyphen in the catalog/database/table name (legal via backticks) gets folded into camelCase and this fallback can never match the key a user would set. The primary kebab lookup at line 286 uses the raw name and is fine. If the camelCase variant is only meant to cover the conf-name part, camel-case just that: sessionConf.get(toCamelCase(sessionConfName) + "." + tableName, null).
|
Thanks for reviewing @wombatu-kun , especially for the context. Having a discussion about #16153 with the community is the most important part. I am open to alternative user-friendly solutions that do not involve code change on the application side or fine tuning individual tables of which it could be hundreds or even thousands. |
|
Hi folks, author of #16185 here - leaving that one as closed and +1 on the general feature added on this PR. Given SQL statements such as MERGE INTO, UPDATE, and DELETE do not expose DataFrame read options, and changing table properties affects all readers, having a session-level override would help standard use cases as well, not only GPU users. I do have other thoughts on naming / interface implementation detail, will leave separate comments |
| public static final String ADVISORY_PARTITION_SIZE = "spark.sql.iceberg.advisory-partition-size"; | ||
|
|
||
| // Overrides the split target size for scan planning | ||
| public static final String SPLIT_SIZE = "spark.sql.iceberg.split-size"; |
There was a problem hiding this comment.
Should this be spark.sql.iceberg.read.split-size instead?
The closest merged precedent is #16088, which added the split-planning session configs under the read namespace:
spark.sql.iceberg.read.adaptive-split-size.enabledspark.sql.iceberg.read.adaptive-split-size.parallelism
Since this is the fixed split-size counterpart to adaptive split sizing, keeping it under spark.sql.iceberg.read.* seems more consistent
| | spark.sql.iceberg.delete-planning-mode | AUTO | Scan planning mode for delete files (`AUTO`, `LOCAL`, `DISTRIBUTED`) | | ||
| | spark.sql.iceberg.advisory-partition-size | Table default | Advisory size (bytes) used for writing to the Table when Spark's Adaptive Query Execution is enabled. Used to size output files | | ||
| | spark.sql.iceberg.split-size | Table default | Overrides `read.split.target-size` for scan planning. Session values are honored like read options and disable adaptive split-size adjustment | | ||
| | spark.sql.iceberg.split-size.<table-name> | Global session default | Table-scoped split size override using the fully qualified table name as a suffix | |
There was a problem hiding this comment.
Could we consider making table-scoped session configs a generic identity-first pattern instead of a split-size-specific suffix? Having the table name at the end of the config seems confusing.
An alternative is to resolve table-scoped session configs as:
spark.sql.iceberg.<catalog>.<namespace...>.<table>.<setting-suffix>
For this config, if the global key becomes spark.sql.iceberg.read.split-size, the table-scoped key would be:
spark.sql.iceberg.<catalog>.<namespace...>.<table>.read.split-size
That keeps the table identity together, then applies the same setting suffix used by the global session config, making it a generic pattern to support table specific session config.
It would be a slightly larger change admittedly, and i'm happy to help or open a separate PR just to add the table level generic session config support if needed.
There was a problem hiding this comment.
I'm not super excited about allowing for table level defaults through the session parameters. Feels like we are working around an issue (the inability to set config per identifier). Are we ok with just allowing the global default here?
The alternative proposed here means we are essentially adding a completely new paradigm to parsing these config entries.
RussellSpitzer
left a comment
There was a problem hiding this comment.
In general when we do modifications to Spark code we only do the latest Spark version first, so review can happen on a single copy of the changes before moving them back in a subsequent PR to other versions.
My larger comment here is that this PR adds the session level split override but also introduces a new mechanism for table scoping session properties. I would put that off for a separate PR because this is a much larger capability that would apply to all sessionConf properties (not just the one being added here) and we should probably have a bit more of a general review on that.
|
Ah I missed - #16154 (comment) which has this same commentary. |
|
@anuragmantri Could you check this out, you are more deeply involved in the Spark code these days than me |
|
I just took a look at the PR and all the historical context. The motivation is clear to me. I echo @RussellSpitzer and @wombatu-kun's comments. The global session conf is the right starting point. Every existing Iceberg session conf ( I'd also suggest removing the table-scoped mechanism from this PR. A few reasons:
I know this design was rejected earlier citing table ambiguity, but since then Spark has added SQL options in SELECT (apache/spark#46707) and INSERT (apache/spark#47591). I think we should add the support for MERGE, UPDATE and DELETE statements as well in Spark. I can attempt to make these changes but I believe that will be the cleanest way to do this. Also, I'm not a GPU programmer but I'm curious, isn't having a uniform split size for all the tables involved in a session be better for GPU bandwidth utilization? |
szehon-ho
left a comment
There was a problem hiding this comment.
my thought can go here as well: #16153 (comment)
|
Thank you for reviews @anuragmantri @liucao-dd @RussellSpitzer @szehon-ho @wombatu-kun Updated the branch to address the review feedback:
Backports or table-scoped behavior can be handled separately if needed. |
My starting point was actually a single global default size. I don't see per-table tuning as a GPU-specific concern. It is like with the CPU the ideal split size depends on compression ratios, table size (e.g. dim vs fact in a join). The GPU specific part is that you often want it much bigger relative to the CPU setting. |
Closes #16153
What changes were made in this PR?
Add a Spark 4.1 session configuration key,
spark.sql.iceberg.read.split-size, that allows overriding theread.split.target-sizetable property at the session level without requiring DDL changes to table metadata or source code changes to read call sites.This is useful when GPU and CPU workloads read the same Iceberg table concurrently: GPU sessions benefit from significantly larger splits while CPU sessions perform better with the default 128MB. Hardware accelerators like RAPIDS Accelerator for Apache Spark are designed as drop-in replacements requiring no application code changes, so a session-level knob is useful.
Changes
Spark 4.1 only:
SparkSQLProperties: addREAD_SPLIT_SIZE = "spark.sql.iceberg.read.split-size".SparkReadConf: add.sessionConf(SparkSQLProperties.READ_SPLIT_SIZE)to bothsplitSize()andsplitSizeOption()parser chains, and document the updated precedence.This PR intentionally does not add a table-scoped session key. Per-read overrides continue to use the existing
split-sizeread option; table-scoped session behavior can be considered separately if needed.Resolution precedence
split-size)spark.sql.iceberg.read.split-size)read.split.target-size)For split size, session overrides are treated like explicit read options and disable adaptive split-size adjustment.
How was this patch tested?
Verified locally against current
apache/main(20bf72be):./gradlew :iceberg-spark:iceberg-spark-4.1_2.13:test --tests org.apache.iceberg.spark.TestSparkReadConf --tests org.apache.iceberg.spark.source.TestSparkScan./gradlew :iceberg-spark:iceberg-spark-4.1_2.13:spotlessCheck./gradlew :iceberg-spark:iceberg-spark-4.1_2.13:checkstyleMain :iceberg-spark:iceberg-spark-4.1_2.13:checkstyleTestgit diff --check