refactor: group orphan spark.comet.explain.* configs under one prefix#5026
Open
andygrove wants to merge 1 commit into
Open
refactor: group orphan spark.comet.explain.* configs under one prefix#5026andygrove wants to merge 1 commit into
spark.comet.explain.* configs under one prefix#5026andygrove wants to merge 1 commit into
Conversation
Three explain-related boolean flags lived at the top level while their
close siblings (`format`, `native.enabled`, `rules`) already sat under
`spark.comet.explain.*`. Move them under the group:
- `spark.comet.explainFallback.enabled`
-> `spark.comet.explain.fallback.enabled`
- `spark.comet.explainCodegen.enabled`
-> `spark.comet.explain.codegen.enabled`
- `spark.comet.logFallbackReasons.enabled`
-> `spark.comet.explain.fallback.log.enabled`
`logFallbackReasons` and `explainFallback` are the two fallback-reason
reporters (aggregated single WARN vs one WARN per reason); putting them
both under `.explain.fallback.*` makes the relationship visible.
Old keys keep working via `.withAlternative(...)` and log a one-time
deprecation warning when read. The `ENABLE_COMET_LOG_FALLBACK_REASONS`
env var is preserved as-is.
The `val COMET_LOG_FALLBACK_REASONS` is renamed to
`COMET_EXPLAIN_FALLBACK_LOG_ENABLED` to match the new key; the two
sibling vals already had matching names.
Part of apache#4978 (Category 3).
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
1 participant
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.
Summary
Part of #4978 (Category 3).
Three explain-related boolean flags lived at the top level while their close
siblings (
format,native.enabled,rules) already sat underspark.comet.explain.*. This PR moves them under the group so the confignamespace is discoverable and consistent.
Rename table
spark.comet.explainFallback.enabledspark.comet.explain.fallback.enabledspark.comet.explainCodegen.enabledspark.comet.explain.codegen.enabledspark.comet.logFallbackReasons.enabledspark.comet.explain.fallback.log.enabledFor the placement question raised by the issue on
logFallbackReasons(either
explain.fallback.log.enabledorfallback.log.enabled), I went withexplain.fallback.log.enabled.logFallbackReasonsandexplainFallbackarethe two fallback-reason reporters — one aggregated WARN vs one WARN per
reason — so nesting the "log every reason" variant under the same
.explain.fallback.*parent makes the pairing visible in tab-completion anddocs.
Every renamed key is registered via
.withAlternative(...), so existing userconfigurations keep working (a one-time deprecation warning is logged per old
key). No behavior change.
Not in this PR
ENABLE_COMET_LOG_FALLBACK_REASONSenv var is preserved as-is. There isno alias mechanism for env vars, and renaming it would silently break users
who set it in their launcher configs.
dev/diffs/iceberg/*.difffiles still reference the old string keys.They keep working through aliases, and the shuffle rename PR (refactor: unify shuffle configs under
spark.comet.shuffle.*prefix #4986) tookthe same approach — regenerating those diffs is out of scope here.
Val rename
val COMET_LOG_FALLBACK_REASONSis renamed toCOMET_EXPLAIN_FALLBACK_LOG_ENABLEDto match the new key (the two siblingvals already had matching names, so only this one moved).
Test plan
./mvnw -Pspark-3.5 test -pl spark -Dsuites="org.apache.comet.CometConfSuite"— 10 tests pass (including 3 new alias tests)./mvnw -Pspark-3.5 -pl spark spotless:check— clean./mvnw -Pspark-3.5 -pl spark test-compile— clean