Skip to content

docs: exempt testing-category and internal configs from the versioning policy - #6089

Open
andygrove wants to merge 3 commits into
apache:mainfrom
andygrove:pr-for-updating-comet-versioning-policy-90e9c441
Open

andygrove wants to merge 3 commits into
apache:mainfrom
andygrove:pr-for-updating-comet-versioning-policy-90e9c441

Conversation

@andygrove

@andygrove andygrove commented Sep 21, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #6088.

Rationale for this change

The versioning policy currently puts every spark.comet.* key inside the compatibility surface, with no carve-out. Read literally, that applies the full contract to configs that exist only so Comet can be tested and debugged: renaming one would need a withAlternative alias, changing a default would need a spark.comet.legacy.* escape hatch plus an upgrade guide entry, and removing one would need a deprecation cycle and a major release.

Two separate mechanisms mark such a key today, and the policy should recognize both.

.category(CATEGORY_TESTING) routes a key into the Development & Testing Settings table instead of in with the production settings. These keys let Comet's own suites, and contributors chasing a bug, reach a state the rest of the code is not built to support — spark.comet.scan.enabled to isolate a planner problem, spark.comet.explain.fallback.strict.enabled to make a declined operator throw rather than quietly report itself, spark.comet.testing.aggregate.partialMode.enabled to run a partial aggregate without its final counterpart, and the on-heap family because Spark's own SQL suite needs on-heap mode.

.internal() goes further: the key is left out of configs.md entirely, so Comet never publishes its name, its default, or what it does. Three keys are internal but not in the testing category — spark.comet.exec.broadcast.enabled ("this config is only used for unit test"), spark.comet.shuffle.jvm.spillThreshold, and spark.comet.shuffle.jvm.memoryWaitTimeout (both "internal config for testing purpose or advanced tuning"). Guaranteeing a key that users have no supported way to discover is a commitment with no corresponding benefit.

Guaranteeing either kind would pin the internals they exist to expose, which is exactly what the rest of the policy leaves free to change. So the policy now states one rule over the two declarations themselves: a key is exempt only if it is in the testing category or is marked internal(), and every other spark.comet.* key is covered. It then says what follows in both directions — maintainers owe the exempt keys nothing, and contributors may not use either mechanism to ship a production knob without committing to it.

The rule is deliberately stated over the source declaration rather than over what configs.md publishes, because that page is not an exhaustive list of covered keys. Two families are covered but have no row on it, as @sunchao caught in review: the per-expression spark.comet.expression.<Name>.allowIncompatible opt-ins, which have no ConfigEntry at all (CometConf.isExprAllowIncompat reads them by string) and are documented in the compatibility guide instead; and the deprecated alias a rename leaves behind, since generateConfigReference writes conf.key and nothing else while this same policy says the alias may only be dropped in a major release. An absence-based rule would have silently exempted both.

What changes are included in this PR?

Documentation only; no code changes.

  • docs/source/about/versioning_policy.md: a new Testing and Internal Configurations Are Exempt subsection under the compatibility surface, giving the rule, the paragraph on why absence from configs.md is not the test, and then each mechanism, what the exemption permits (change or removal in any release, with no alias, no legacy config, no deprecation cycle, no upgrade guide entry), why, and the constraint it places on contributors. The covered/not-covered bullet lists are updated, and the three sections that previously implied the opposite now cross-reference it: patch releases (which said they add no configuration keys), behavior changes, and renames.
  • docs/source/contributor-guide/config_conventions.md: Categories becomes Categories and Visibility and gains a table mapping (category, internal()) to whether the key is published and whether it is covered, so a contributor can answer the question at a glance, plus the corollary that a key read by string cannot carry either mark and is therefore covered — which closes the obvious way around the guarantee. The rename checklist and the behavior-change section note the exemption.
  • docs/source/user-guide/latest/configs.md: a note above the Development & Testing Settings table, plus a paragraph saying that internal keys are deliberately absent from the page and carry no guarantee, while absence alone does not imply that — the allowIncompatible opt-ins are documented in the compatibility guide and are covered like any other production setting.

How are these changes tested?

Not applicable — documentation only. prettier --check passes on the three files, and every relative link and heading anchor added here resolves against an existing heading. The claims about which keys are affected were taken from CometConf.scala and GenerateDocs.scala rather than from the prose: 16 keys carry CATEGORY_TESTING, 7 carry .internal(), 4 carry both, every ConfigEntry declares a category, GenerateDocs filters on isPublic so internal() is what keeps a key off the page, and generateConfigReference emits only conf.key, which is why a withAlternative alias never gets a row.

@andygrove andygrove added the documentation Improvements or additions to documentation label Sep 21, 2026
@andygrove andygrove changed the title docs: exempt testing-category configs from the versioning policy docs: exempt testing-category and internal configs from the versioning policy Sep 21, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness

The current policy covers every spark.comet.* key. This PR proposes an explicit exemption for testing-category and internal configurations, allowing changes or removal in any release without aliases, legacy settings, a deprecation cycle or an upgrade-guide entry. It updates the versioning policy, contributor conventions and the user-facing testing section together.

The two classifications are independent in the implementation: category(...) selects a table, while internal() sets isPublic=false. I verified the stated inventory of 16 testing-category entries, seven internal entries and four in both groups. The existing memory-management guide already excludes on-heap mode from production use. The added reference-page prose sits outside generated markers, so regeneration preserves it.

[P2] The exhaustive reference-page rule exempts existing public keys beyond those two groups. Per-expression allowIncompatible settings are documented in the compatibility guide and used by the planner, but are read as dynamic string keys and are absent from configs.md. The inline comment gives the concrete example and a narrower policy definition. Deprecated public aliases are also omitted from generated key rows despite their separate retention guarantee.

This is a documentation change, so it does not change Spark expression results, null handling, errors or fallback at runtime. I checked the maintained Spark 3.5 and 4.0 SQLConf.getConfString implementations: explicit string settings do not need a registered config entry, which confirms that these dynamic Comet keys remain usable. Maintained 3.4 and 4.1 sources were unavailable, and I make no source-coverage claim for them.

Validation

The current checks show seven successes and 15 skips. The Preflight log verifies checkout of merge 0191caa0, with parents 09b44ad6 and e74c0345, and a successful prettier --check "**/*.md". The merge tree and all three changed files match the reviewed head. Local validation covers source classification, generated-content boundaries, links and the exact diff. Product suites were skipped for this documentation-only change. No local product build, runtime test or benchmark was run.

Performance

The PR changes no executable code, allocation, planner work or query path. Its performance effect is limited to future policy decisions about testing controls, which this diff does not implement. A runtime microbenchmark would not validate the policy boundary at issue.

Design

Using existing testing and visibility metadata is a reasonable way to keep implementation-facing controls outside the compatibility contract. The contributor rule also correctly requires legitimate production controls to remain public. The policy should express those explicit classifications without inferring internal status from absence on one documentation page. Public dynamic keys and separately documented configuration surfaces need to retain their intended contract.

I read the #6066 compatibility review and author response as background. The previously merged rule and this proposed exemption are different policies. If this proposal lands, its testing/internal exemption changes the basis for that discussion. It does not establish that the exemption already exists today.

Abstraction & complexity

No new product abstraction is introduced. The category/visibility table makes the two existing dimensions easy to inspect, and cross-references keep the rename and behavior-change guidance connected to the main policy. The simplest correction is to narrow the exemption to the explicitly classified keys and preserve the existing public configuration families. A new registry or runtime classification mechanism is unnecessary for this change.

Comment thread docs/source/about/versioning_policy.md Outdated
Comment on lines +81 to +84
Two kinds of configuration key sit outside this policy, and one rule identifies both: **a key is
covered only if the [configuration reference](../user-guide/latest/configs.md) lists it outside the
Development & Testing Settings table.** Everything the reference publishes as a production setting
is guaranteed; nothing else is.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness

[P2] Preserve documented expression opt-ins in the compatibility surface

Could the exemption be defined by an explicit testing category or internal() designation, while retaining public keys documented outside configs.md? For example, spark.comet.expression.RLike.allowIncompatible is documented in the compatibility guide and read by CometConf.isExprAllowIncompat, but it has no ConfigEntry and is absent from the generated configuration table. GenerateDocs emits per-expression .enabled keys there and emits native opt-ins on the compatibility pages instead. This rule therefore exempts an existing user-facing config family that is neither testing nor internal, permitting its keys/defaults to change without the normal compatibility path. The same exhaustive statement at configs.md:83-85 needs to use the narrower definition too. Deprecated public aliases are also omitted from the generated key rows even though this policy retains them until a major release.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, and you're right on both counts. I checked before changing anything: GenerateDocs.generateConfigReference writes conf.key and nothing else, so a deprecated alias never gets a row even though this same policy says the alias may only be dropped in a major release — the rule as I wrote it contradicted the rename section two screens further down. And the per-expression allowIncompatible keys have no ConfigEntry at all; CometConf.isExprAllowIncompat reads them by string, so nothing generates them into a table. They are documented in the compatibility guide, and users set them in production to accept a known divergence from Spark, which is exactly the kind of key the policy should be pinning.

The mistake was reaching for a documentation artifact as the test when the thing I actually meant is declared in the source. So the rule is now stated over the two marks directly — a key is exempt only if it is in the testing category or is marked internal(), and every other spark.comet.* key is covered — with an explicit paragraph saying absence from configs.md is not the test, naming the allowIncompatible opt-ins and the deprecated alias as the cases that prove it. The covered bullet at the top now says the same thing positively, and the note at configs.md:83-85 narrows to internal keys and points at the compatibility guide for the opt-ins.

One consequence worth making explicit, so I added it to the contributor guide: a key read by string cannot carry either mark, so under the new rule it is covered by default. That closes the obvious way around the guarantee, which the old phrasing left wide open — a dynamic key was automatically exempt.

The exemption was phrased as an exhaustive rule over the configuration
reference: a key was covered only if configs.md published it as a
production setting. That over-reached. Two families of covered key have
no row on that page.

The per-expression spark.comet.expression.<Name>.allowIncompatible
opt-ins have no ConfigEntry at all - CometExecRule and QueryPlanSerde
read them by string - so GenerateDocs never emits them. They are
documented in the compatibility guide instead, and users set them in
production to accept a known divergence from Spark. A deprecated alias
left behind by withAlternative has no row either, because
generateConfigReference writes conf.key and nothing else, yet this same
policy says an alias may only be dropped in a major release.

So state the rule over the two declarations that actually mark a key,
.category(CATEGORY_TESTING) and .internal(), and say outright that
absence from configs.md is not the test. The contributor guide gains the
corollary that a key read by string cannot carry either mark and is
therefore covered, which closes the obvious way around the guarantee.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exempt testing-category and internal configs from the versioning policy

2 participants