andygrove commented on code in PR #6089: URL: https://github.com/apache/datafusion-comet/pull/6089#discussion_r4072978967
########## docs/source/about/versioning_policy.md: ########## @@ -71,6 +76,46 @@ 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 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. Review Comment: 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
