This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-6089-29d5d4f939bd340ab7107bcf6d7ca00f541e8dfe in repository https://gitbox.apache.org/repos/asf/datafusion-comet.git
commit 8166586fa767008a45665358d200ce23ec955f64 Author: Andy Grove <[email protected]> AuthorDate: Thu Sep 24 09:38:02 2026 +0000 docs: exempt testing-category and internal configs from the versioning policy (#6089) * docs: exempt testing-category configs from the versioning policy * docs: extend the config exemption to internal() keys * docs: key the config exemption on the declaration, not on configs.md 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. --- docs/source/about/versioning_policy.md | 73 +++++++++++++++++++++- .../source/contributor-guide/config_conventions.md | 48 +++++++++++++- docs/source/user-guide/latest/configs.md | 13 ++++ 3 files changed, 130 insertions(+), 4 deletions(-) diff --git a/docs/source/about/versioning_policy.md b/docs/source/about/versioning_policy.md index e5cb832f44..7b6e4974c8 100644 --- a/docs/source/about/versioning_policy.md +++ b/docs/source/about/versioning_policy.md @@ -48,7 +48,14 @@ interface that vendors implement. It is small on purpose, and everything outside The following are covered by this versioning policy: - **Configuration keys under `spark.comet.*`**: their names, types, accepted values, default - values, and semantics. + values, and semantics. Two classes of key are excluded — those in the `testing` category, and + those marked internal; see + [Testing and Internal Configurations Are Exempt](#testing-and-internal-configurations-are-exempt). + Every other key is covered wherever Comet documents it, including keys the + [configuration reference](../user-guide/latest/configs.md) does not list in its own right: the + per-expression `spark.comet.expression.<Name>.allowIncompatible` opt-ins, which the + [compatibility guide](../user-guide/latest/compatibility/index.md) documents, and the deprecated + alias a rename leaves behind. - **A small, enumerated public Java and Scala API**: the class names users write into Spark config properties, and the S3 credential provider SPI that vendors implement. The full list is in [Public Scala and Java API](#public-scala-and-java-api). @@ -59,6 +66,9 @@ The following are covered by this versioning policy: The following are internal implementation details. They are not covered by this policy and may change in any release: +- Configuration keys in the `testing` category, and keys marked internal. They exist to let Comet's + own suites and maintainers reach internal behavior, and they expose that behavior directly. See + [Testing and Internal Configurations Are Exempt](#testing-and-internal-configurations-are-exempt). - The protobuf format used to serialize query plans between the JVM and the native library. The JVM jar and the native library ship together and are versioned together; see [Native Library Coupling](#native-library-coupling). @@ -71,6 +81,52 @@ change in any release: fall back to Spark. An expression that ran natively in one release may fall back in the next, and vice versa. The results stay the same; only the speed changes. +### Testing and Internal Configurations Are Exempt + +Two kinds of configuration key sit outside this policy, and what puts a key outside is an explicit +mark on its declaration in `CometConf.scala`: **a key is exempt only if it is in the `testing` +category or is marked `internal()`.** Every other `spark.comet.*` key is covered. + +Absence from the [configuration reference](../user-guide/latest/configs.md) is not the test, +because that page is not an exhaustive list of covered keys. The per-expression +`spark.comet.expression.<Name>.allowIncompatible` opt-ins are read as plain strings with no +`ConfigEntry` behind them, so no generated table ever lists them; the +[compatibility guide](../user-guide/latest/compatibility/index.md) is where they are documented +instead. A deprecated alias left behind by a rename has no row of its own either. Both are fully +covered. + +The two mechanisms that do exempt a key are set independently of each other: + +- **The `testing` category.** Every key declares a category, and `testing` routes it into the + Development & Testing Settings table rather than in with the production settings. These keys + exist so that Comet's own suites, and contributors chasing a bug, can reach a state the rest of + the code is not built to support: disabling native scans to isolate a planner problem, running + Comet in on-heap mode, making a declined operator throw instead of quietly reporting itself, or + running a partial aggregate without its final counterpart. Setting one is a debugging step, not a + deployment choice. +- **`internal()`.** A key marked internal is left out of the configuration reference entirely, so + Comet never publishes its name, its default, or what it does. A user has no supported way to + learn that it exists, which is the point: these are escape hatches a maintainer reaches for while + working on Comet itself. + +For a key caught by either mechanism, the name, type, accepted values, default value, and semantics +may change in any release, including a patch release, and the key may be removed outright. None of +the machinery the rest of this policy requires applies: no `spark.comet.legacy.*` escape hatch for a +behavior change, no deprecated alias for a rename, no deprecation cycle before removal, and no +upgrade guide entry. + +The exemption exists because these keys point at Comet's internals by construction. Guaranteeing +them across releases would pin the implementation details they expose, which is the thing the rest +of this section deliberately leaves free to change. + +The corollary binds contributors: **neither mechanism may be the only way to reach a behavior that +production users need.** If a knob turns out to be one that deployments legitimately set, it belongs +in a non-`testing` category and must not be marked internal, and the guarantees come with it. +Choosing a category, and deciding whether to mark a key internal, are therefore policy decisions +rather than routing details; see +[Categories and Visibility](../contributor-guide/config_conventions.md#categories-and-visibility) +in the contributor guide. + ## What Each Version Component Means ### Major Releases @@ -99,8 +155,11 @@ A minor release may: ### Patch Releases A patch release contains bug fixes only. It adds no configuration keys and makes no behavior -changes, with one exception: correctness fixes, which are covered in -[Correctness Fixes Are Not Breaking Changes](#correctness-fixes-are-not-breaking-changes). +changes, with two exceptions: correctness fixes, which are covered in +[Correctness Fixes Are Not Breaking Changes](#correctness-fixes-are-not-breaking-changes), and +configuration keys in the `testing` category or marked internal, which are outside the policy +altogether and may be added, changed, or removed in any release. See +[Testing and Internal Configurations Are Exempt](#testing-and-internal-configurations-are-exempt). ## Behavior Changes and Legacy Configurations @@ -125,6 +184,10 @@ Behavior changes that require this treatment include changing the default value configuration key, changing what an existing key's values mean, and changing the semantics of an `Incompatible` expression or operator whose divergence from Spark users may have come to depend on. +Changing a `testing` or internal key is not a behavior change for this purpose, and needs no escape +hatch. See +[Testing and Internal Configurations Are Exempt](#testing-and-internal-configurations-are-exempt). + ### Lifetime of a Legacy Configuration A `spark.comet.legacy.*` key is deprecated from the moment it is added. Its purpose is to buy users @@ -147,6 +210,10 @@ The alias may only be dropped in a major release. Removing a configuration key outright requires a deprecation cycle: the key must remain available, with a deprecation warning, for at least one minor release before it is removed in a major release. +Neither rule applies to a `testing` or internal key, which may be renamed without an alias and +removed in any release. See +[Testing and Internal Configurations Are Exempt](#testing-and-internal-configurations-are-exempt). + ## Correctness Fixes Are Not Breaking Changes Comet's contract is to produce the results that Apache Spark produces. When an expression or diff --git a/docs/source/contributor-guide/config_conventions.md b/docs/source/contributor-guide/config_conventions.md index 5bcad087ea..6c198f740c 100644 --- a/docs/source/contributor-guide/config_conventions.md +++ b/docs/source/contributor-guide/config_conventions.md @@ -66,19 +66,60 @@ The symbol name is what appears in code; the key is what appears in user configu The two do not have to match segment-for-segment — brevity in the symbol is fine as long as the key remains descriptive. -## Categories +## Categories and Visibility Every `ConfigEntry` must call `.category(...)`. The category is used to route the key into the right table in the user guide's `configs.md`. Available categories are declared as `CATEGORY_*` constants at the top of `CometConf.scala`. If a new config does not fit an existing category, discuss adding a new one before landing the config. +A second, independent choice is whether to call `.internal()`, which keeps the key out of +`configs.md` altogether. + +Neither choice is cosmetic. Together they decide whether Comet's +[versioning policy](../about/versioning_policy.md) covers the key. The policy's rule keys on these +two marks and nothing else: a key is exempt only if it is in the `testing` category or is marked +`internal()`, and every other `spark.comet.*` key is covered. + +| Category | `internal()` | Appears in `configs.md` | Covered by the versioning policy | +| --------- | ------------ | -------------------------------- | -------------------------------- | +| any other | no | yes, in its category's table | **yes** | +| `testing` | no | yes, under Development & Testing | no | +| any | yes | no | no | + +Note that a key read by string rather than through a registered `ConfigEntry` — as the +per-expression `spark.comet.expression.<Name>.allowIncompatible` opt-ins are — has no way to carry +either mark, so it is covered. Reaching for a dynamic key is therefore not a way to avoid the +guarantee; if the key is a debugging aid, give it a real `ConfigEntry` in the `testing` category. + +Being covered commits the project to the key's name, type, accepted values, default, and semantics +across minor releases. Being +[exempt](../about/versioning_policy.md#testing-and-internal-configurations-are-exempt) means the key +may be renamed, retyped, redefaulted, or removed in any release, including a patch release, with no +alias, no deprecation cycle, and no upgrade guide entry. + +So reach for `testing` or `internal()` only when the key exists to let Comet's own suites, or +someone debugging Comet itself, reach a state the rest of the code is not built to support — +disabling native scans, running in on-heap mode, making a declined operator throw. Use `internal()` +in particular when Comet should not publish the key at all, so that nobody can adopt it in the first +place. + +If a deployment would have a legitimate reason to set the key, it belongs in a non-`testing` +category and must not be `internal()`, and the guarantees come with it. Do not use either mechanism +as a way to ship a production knob without committing to it. + ## Renaming an Existing Config Configs under `spark.comet.*` are stable across minor releases: users may have set them in production `spark-defaults.conf` files, Spark job submissions, or notebooks. Renaming a key must not silently break those deployments. +Keys in the `testing` category, and keys marked `internal()`, are +[exempt from the versioning policy](../about/versioning_policy.md#testing-and-internal-configurations-are-exempt) +and may be renamed outright: skip the `withAlternative` call in step 1, and step 5 with it. The +rest of the checklist still applies, because a stale key string left behind in code or docs is a +bug either way. + Use the `withAlternative` builder on `ConfigBuilder` to keep the old key working as a deprecated alias: @@ -159,5 +200,10 @@ Two cases do **not** need a legacy config: - **Changes to which expressions and operators run natively.** Falling back to Spark, or ceasing to, changes performance rather than results. +Nor does a change to a key in the `testing` category or a key marked `internal()`, both +[exempt from the versioning policy](../about/versioning_policy.md#testing-and-internal-configurations-are-exempt): +the default and the meaning may change in any release, with no legacy config and no upgrade guide +entry. Update the suites that set it in the same PR. + Removing a legacy config is a major-release change, handled the same way as removing a deprecated alias. diff --git a/docs/source/user-guide/latest/configs.md b/docs/source/user-guide/latest/configs.md index 49ee6dd48d..59d5c711fa 100644 --- a/docs/source/user-guide/latest/configs.md +++ b/docs/source/user-guide/latest/configs.md @@ -74,6 +74,19 @@ shuffle, including applications that rely only on remote reliable storage or dec ## Development & Testing Settings +These settings exist for Comet's own test suites and for debugging. They are **not covered by the +[versioning policy](../../about/versioning_policy.md#testing-and-internal-configurations-are-exempt)**: +their names, defaults, accepted values, and meanings may change in any release, including a patch +release, and any of them may be removed without a deprecation cycle. Do not set them in +production. + +Comet also marks a handful of keys internal and deliberately leaves them off this page entirely. +They are maintainer escape hatches, not settings, and carry no guarantee of any kind. Absence from +this page does not by itself mean that, though: the per-expression +`spark.comet.expression.<Name>.allowIncompatible` opt-ins are documented in the +[compatibility guide](compatibility/index.md) rather than here, and the versioning policy covers +them like any other production setting. + <!--BEGIN:CONFIG_TABLE[testing]--> <!--END:CONFIG_TABLE--> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
