[AURON #2360] Honor config alt keys when reading from SQLConf#2361
Open
robreeves wants to merge 6 commits into
Open
[AURON #2360] Honor config alt keys when reading from SQLConf#2361robreeves wants to merge 6 commits into
robreeves wants to merge 6 commits into
Conversation
Auron's ConfigOption alt keys (declared via addAltKey) were silently ignored: getFromSpark only consulted alt keys via ConfigEntry.findEntry (always null for Auron's unregistered options) and then synthesized a ConfigEntryWithDefaultFunction with an empty alternatives list, so only the primary key was ever read from SQLConf. As a result, e.g. setting spark.auron.enable (alt of spark.auron.enabled) had no effect. Pass the spark-prefixed alt keys as the synthesized entry's alternatives so ConfigEntry#readString reads primary +: alternatives, with the primary key taking precedence. Also add a test asserting alt keys are honored. Fixing this makes the test harness's spark.auron.enable=false baseline actually fall back to vanilla Spark, which exposed that acosh(0.0) yields NaN with a different (implementation-defined) bit pattern in each engine; QueryTest compares doubles via Double.doubleToRawLongBits, so update the acosh test to assert NaN-ness for the out-of-domain input rather than exact equality. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
QueryTest compares doubles via Double.doubleToRawLongBits, which is bit-exact. Vanilla Spark and the native engine can produce semantically equal NaNs with different (implementation-defined) bit patterns, so the comparison would spuriously fail. Canonicalize NaN on both sides before comparing. This lets the acosh null propagation test keep its original single-query form covering the out-of-domain (NaN) input. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Auron’s Spark SQLConf integration so ConfigOption.addAltKey(...) aliases are actually consulted when reading configuration values, and updates the test harness to reflect the now-correct behavior.
Changes:
- Populate synthesized
ConfigEntryWithDefaultFunctionalternatives with spark-prefixed alt keys so SQLConf readsprimary + alternatives. - Add a unit test validating alt-key behavior and primary-key precedence for
AURON_ENABLED. - Canonicalize NaN values in query-result comparisons to avoid false mismatches from differing NaN bit patterns.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| spark-extension/src/main/java/org/apache/auron/spark/configuration/SparkAuronConfiguration.java | Synthesizes Spark ConfigEntry with alt keys so SQLConf reads both primary and aliases. |
| spark-extension-shims-spark/src/test/scala/org/apache/spark/sql/AuronQueryTest.scala | Canonicalizes NaN before comparing Spark-vs-Auron results to avoid raw-bit NaN mismatches. |
| spark-extension-shims-spark/src/test/scala/org/apache/auron/AuronQuerySuite.scala | Adds test coverage ensuring config alt keys are honored and primary key wins on conflicts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…cker Revert checkSparkAnswerAndOperator to plain checkAnswer and instead handle the NaN encoding difference locally in the acosh test. acosh of an out-of-domain input yields NaN, which vanilla Spark and the native engine may encode with different bits; checkAnswer/QueryTest compares doubles by raw bits. Split the test so in-domain/null values are compared numerically, and out-of-domain inputs are compared via the natively-supported isnan (a boolean) so no raw NaN bits are compared. This keeps the shared checker unchanged and avoids relaxing NaN comparison for all callers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
With the config alt-key fix, checkSparkAnswerAndOperator's baseline now actually runs vanilla Spark. On Spark 4.x ANSI mode is enabled by default, so negating Int.MinValue throws ARITHMETIC_OVERFLOW in vanilla Spark while the native engine wraps, causing the comparison to diverge (and the test to fail only on the spark-4.0/4.1 CI jobs). Run the test with spark.sql.ansi.enabled=false so both engines wrap consistently while still exercising the Int.MinValue boundary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #2360
Rationale for this change
Config aliases declared via
ConfigOption.addAltKey(...)were silently ignored: only the primary key was ever read from Spark'sSQLConf, so every alternative key inSparkAuronConfigurationhad no effect. For example, settingspark.auron.enable(the alt key ofspark.auron.enabled) did not disable Auron.The cause is in
SparkAuronConfiguration.getFromSpark(...): alt keys were only resolved viaConfigEntry.findEntry(...)against Spark's global registry. Auron's options are not registered there, so the lookup always fell back to synthesizing aConfigEntryWithDefaultFunctionkeyed on the primary key with an empty alternatives list, andConfigEntry#readStringtherefore only read the primary key.What changes are included in this PR?
ConfigEntryWithDefaultFunction'salternativessoConfigEntry#readStringreadsprimaryKey +: alternatives, with the primary key taking precedence.AuronQuerySuite."config alt keys are honored") asserting both primary and alt keys take effect forAURON_ENABLED, and that the primary key wins when both are set.acosh null propagationtest. With alt keys now honored, the test harness'sspark.auron.enable=falsebaseline actually falls back to vanilla Spark, which exposed that vanilla Spark and the native engine can produce the same NaN with different bit patterns (checkSparkAnswerAndOperator/QueryTestcompares doubles viaDouble.doubleToRawLongBits). The test now keeps the exact numeric/null comparison for in-domain inputs, and compares the out-of-domain (NaN-producing) inputs via the natively-supportedisnanso no raw NaN bits are compared. The sharedcheckSparkAnswerAndOperatorchecker is left unchanged.Are there any user-facing changes?
Yes. Configuration alternative keys now work as documented. In particular,
spark.auron.enable(alias ofspark.auron.enabled) andspark.auron.ui.enable(alias ofspark.auron.ui.enabled) now take effect. The primary key takes precedence when both are set.How was this patch tested?
AuronQuerySuite."config alt keys are honored".spark-extension-shims-sparktest suite (Spark 3.5, Scala 2.12): 138 succeeded, 0 failed.