-
Notifications
You must be signed in to change notification settings - Fork 241
[AURON #2295] Replace hardcoded config strings with typed key() references - #2296
[AURON #2295] Replace hardcoded config strings with typed key() references #2296guixiaowen wants to merge 4 commits into
Conversation
guixiaowen
commented
Jun 1, 2026
@cxzl25 Could you please help review another issue, #2252? I noticed that many places in the codebase are still using hardcoded config strings instead of the corresponding .key() values. So I've made changes to unify them. Please take a look when you have a chance. Thank you very much for your help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Refactors several user-facing "conversion disabled / set config ..." messages to avoid hardcoded spark.auron.* configuration key strings by referencing SparkAuronConfiguration typed config options, aligning with issue #2295’s goal of safer config key usage.
Changes:
- Updated conversion-disabled assertion text in Iceberg conversion path to reference
SparkAuronConfiguration. - Updated shuffle manager requirement message to reference
SparkAuronConfiguration. - Updated multiple "Conversion disabled ..." and "Set ... to enable ..." messages in
AuronConvertersto referenceSparkAuronConfiguration.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 16 comments.
| File | Description |
|---|---|
| thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergConvertProvider.scala | Adjusts conversion-disabled message to reference typed config option. |
| spark-extension/src/main/scala/org/apache/spark/sql/auron/AuronSparkSessionExtension.scala | Adjusts shuffleExchange requirement message to reference typed config option. |
| spark-extension/src/main/scala/org/apache/spark/sql/auron/AuronConverters.scala | Adjusts multiple conversion-disabled/config-hint messages to reference typed config options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cxzl25
commented
Jun 1, 2026
Would it be better if there was a public method that could splice Spark prefixed keys?
... references
... references
... references
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
... references
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still hardcodes the spark. prefix, where the rest of the PR routes through the new helper (conversionDisabledMsg → sparkKey()). Two siblings do the same — AuronConverters.scala:509 (ENABLE_SCAN_ORC_TIMESTAMP) and AuronSparkSessionExtension.scala:77 (ENABLE_SHUFFLE_EXCHANGE).
Since #2295 is about removing hardcoded spark.* key strings, would it make sense to route all three through SparkAuronConfiguration.sparkKey(...) as well, so the literal spark. lives in exactly one place? sparkKey is public static and already called from Scala in conversionDisabledMsg, so it'd be a small change — e.g. s"Set ${SparkAuronConfiguration.sparkKey(ENABLE_SCAN_PARQUET_TIMESTAMP)}=true ...".
guixiaowen
commented
Jun 24, 2026
Would it be better if there was a public method that could splice Spark prefixed keys?
@cxzl25 c Thank you very much for the code review. I've been a bit busy these past couple of days, but I will complete the revisions as soon as possible. Thank you again for reviewing the code.
...ences
Which issue does this PR close?
Closes #2295
Rationale for this change
Background
Currently, in the AURON codebase, many feature flag configurations (e.g., auron.enable.hudi.scan, auron.enable.orc.scan, etc.) are referenced as hardcoded strings scattered throughout the implementation code.
For example:
`
case _: FileSourceScanExec if !enableScan =>
`
Problem
Using hardcoded strings for configuration keys introduces several issues:
Maintainability risk – If a configuration key name needs to be changed, developers must manually search and replace all occurrences, which is error-prone.
Typo vulnerability – Hardcoded strings are prone to spelling mistakes that cannot be caught at compile time (e.g., "auron.enable.hu di.scan" with an extra space).
Code inconsistency – Some parts of the codebase use the typed .key() approach, while others still rely on raw strings, leading to inconsistent coding patterns.
Refactoring difficulty – IDE refactoring tools cannot automatically rename hardcoded string literals, making large-scale changes tedious and risky.
What changes are included in this PR?
Scope of Changes
This refactoring should cover all auron.enable.* configuration keys in the codebase, including but not limited to:
auron.enable.hudi.scan
auron.enable.orc.scan
auron.enable.parquet.scan
Any other auron.enable.* feature flags currently defined as typed constants
Are there any user-facing changes?
Nothing.
How was this patch tested?
UT.