tlfua opened a new pull request, #7568:
URL: https://github.com/apache/datafusion-comet/pull/7568
## Which issue does this PR close?
Closes #6767.
## Rationale for this change
Spark accepts any `spark.comet.*` key, so a typo such as
`spark.comet.exec.enabeld` is silently ignored and the user gets default
behavior without knowing why. This PR logs a warning for keys that Comet does
not recognize, with a suggestion when a known key is close.
### Exact check vs. pattern check for dynamic keys
Some keys embed a name, such as `spark.comet.expression.<Name>.enabled` and
`spark.comet.operator.<Name>.allowIncompatible`. There are two ways to validate
them:
| | Exact check (this PR) | Pattern check |
|---|---|---|
| How | `<Name>` must be an expression/aggregate with a serde, or the key
must be registered (operators) | Any key that matches
`spark.comet.expression.*.enabled` etc. is accepted |
| Typo in the name (`Castt`, `WindowExec` after it stopped being
`Incompatible`) | Caught, with a suggestion | Missed: the key is silently
ignored, so the user gets default behavior and no hint why |
| False positive | Possible if a future or out-of-repo contrib operator
returns `Incompatible` without registering its key. The warning is then
misleading, but the key still works, because `isOperatorAllowIncompat` reads
the value without checking registration | None |
| Expression supported only by some Spark versions | Warns on versions where
Comet has no serde for it, which is accurate there (the key has no effect on
that version) | Accepted |
| Maintenance | New `Incompatible` operators must register their key
(documented in `adding_a_new_operator.md`) | None |
A false positive costs one misleading log line and never changes query
behavior. A missed typo is the exact problem the issue asks to fix. So this PR
uses exact checks everywhere. The only pass-through is
`spark.comet.datafusion.*`: those keys are forwarded to DataFusion as they are,
and Comet has no list of valid DataFusion keys.
## What changes are included in this PR?
- New `CometConfValidator`. For each explicitly set `spark.comet.*` key that
is not recognized, it logs one driver-side warning per JVM:
- `Comet configuration 'spark.comet.exec.enabeld' is not recognized. Did
you mean 'spark.comet.exec.enabled'?`
- `Comet configuration 'spark.comet.foo.bar' is not recognized. Check the
key for typos.` (when no known key is within distance 3)
- The suggestion is the known key with the smallest case-insensitive
Levenshtein distance (`UTF8String.levenshteinDistance`), with ties going to the
alphabetically first key. Values are never logged.
- A key is recognized when it is:
- a registered config in `CometConf.allConfs` or from a
`CometConfigProvider` (contrib), including its alternative (deprecated) keys;
- `spark.comet.expression.<Name>.enabled` / `.allowIncompatible` where
`<Name>` is an expression or aggregate that has a serde in `QueryPlanSerde`;
- `spark.comet.version`, which the driver plugin sets itself.
- Operator `allowIncompatible` keys are only read when an operator serde
returns `Incompatible`, and today only `DataWritingCommandExec` and
`WriteFilesExec` do; both keys are registered. So operator keys are checked
exactly against the registry, with no pattern special case.
- `spark.comet.datafusion.*` keys are passed to DataFusion as they are
(`CometExecIterator`), so they are not checked.
- Hooks:
- `CometDriverPlugin.init` checks the `SparkConf`.
- `CometExecRule._apply` checks the session conf, before any early return,
so a typo that ends up disabling Comet is still reported.
- Each key is checked once per JVM. If building the known-key set fails,
the check disables itself (debug log) and never fails SparkContext startup or
planning.
- Removed test/benchmark configs that set keys Comet no longer reads (they
would now warn):
- `spark.comet.operator.WindowExec.allowIncompatible` in
`CometWindowExecSuite`, `lag_lead.sql` and `CometSlidingSumBenchmark`. Window
has not returned `Incompatible` since
https://github.com/apache/datafusion-comet/pull/4209.
- `spark.comet.exec.shuffle.fallbackToColumnar` in `CometAggregateSuite`.
- Docs:
- `configs.md`: describes the warning.
- `adding_a_new_operator.md`: an operator that can return `Incompatible()`
must register its `allowIncompatible` key with `createOperatorIncompatConfig`
(contrib via `CometConfigProvider`), otherwise Comet warns about the key.
Note: https://github.com/apache/datafusion-comet/pull/7567 touches the same
spots in `Plugins.scala` and `CometExecRule._apply`; whichever lands second
will need a small rebase.
## How are these changes tested?
- New unit tests in `CometConfSuite`: suggestions (case, typo, expression
name, no close key), registered/alternative/`spark.comet.version` keys,
expression and operator keys, and keys that are not checked
(`spark.comet.datafusion.*`, non-Comet keys).
- New `CometConfValidatorSuite`: warns once across multiple queries, warns
even when `spark.comet.enabled=false`, and does not warn for recognized keys
(log appender based).
- `make format`: passed.
- `npx prettier --check` on the two changed docs: passed.
- JVM suites: fork CI
https://github.com/tlfua/datafusion-comet/actions/runs/42436925566 (in
progress; will update).
--
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]
