andygrove commented on issue #4978: URL: https://github.com/apache/datafusion-comet/issues/4978#issuecomment-5152179720
Closing this out ahead of 1.0. Most of the audit landed; the remainder is deliberately **will not do**. ## Done - **Category 1** — `spark.comet.exec.replaceSortMergeJoin` → `spark.comet.exec.forceShuffledHashJoin` (the pilot rename). - **Category 2** — the dot-separated keys are gone: no `spark.comet.columnar.shuffle.async.*` or `spark.comet.parquet.read.parallel.io.thread-pool.size` remain. The UDF casing split is fixed in #5197 (`pyarrowUdf` → `pyarrowUDF`, no alias needed — the key never shipped). - **Category 3** — `spark.comet.explain.*` grouped in #5026. - **Category 4** — shuffle prefixes unified under `spark.comet.shuffle.*` in #4986. Also folded into #5197: `config_conventions.md` had drifted from the code (it still listed `columnar` as a category segment, and its symbol-naming table named a symbol that does not exist). ## Will not do **Category 5 — `spark.comet.sparkToColumnar.*`.** The two candidate targets both have real problems. Folding into `spark.comet.convert.*` overloads a prefix that currently means source-scan conversion (`convert.parquet`, `convert.json`, `convert.csv`) with a different concept, converting arbitrary Spark operators to Arrow. Renaming the segment to `sparkColumnar` is churn for pure aesthetics. Not a call worth making on release week. **Category 6 — top-level scalars.** These are not inconsistencies: - `spark.comet.memoryOverhead` deliberately mirrors Spark's own `spark.executor.memoryOverhead`. - `spark.comet.maxTempDirectorySize` mirrors DataFusion's `max_temp_directory_size`. - `spark.comet.batchSize` is probably the most widely cited Comet config in tuning guides, blog posts, and benchmark scripts. **`spark.comet.exceptionOnDatetimeRebase`.** Restructuring a shipped, compatibility-sensitive key is not worth it. Appending `.enabled` alone is cosmetic. **`spark.comet.scan.allowDisabledParquetVectorizedReader`.** Already compliant — the conventions guide exempts action-form `allow…`/`force…` flags from the `.enabled` suffix. ## The reasoning on timing Under the [versioning policy](https://datafusion.apache.org/comet/about/versioning_policy.html), config keys are Comet's primary public API. An alias added now and one added in 1.3 are both removable only in 2.0, so alias lifetime is not the deciding factor — user churn is. Renames at the 0.x→1.0 boundary are one migration, documented once. The same renames mid-1.x hit users who already migrated with fresh deprecation warnings, against names by then baked into 1.0-era docs and blog posts. That makes the cheap renames worth doing now and the expensive ones not worth doing at all, rather than leaving them open as "post-1.0". Two smaller `.enabled` suffix gaps remain unaddressed (`spark.comet.nativeLoadRequired`, `spark.comet.exec.strictFloatingPoint`). They are legitimate but low-value, and both would now need aliases. Happy to reopen as a focused issue if anyone feels strongly. -- 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]
