Repository navigation
Conversation
Log one warning per JVM for each explicitly set spark.comet.* key that Comet does not recognize, with a "Did you mean" hint when a known key is within Levenshtein distance 3. Keys are checked at driver plugin init and at the start of CometExecRule, before any early return, so a typo that disables Comet is still reported. spark.comet.datafusion.* keys are passed through to DataFusion and are not checked. Also removes test configs that set keys Comet no longer reads. Co-authored-by: Copilot <[email protected]> Copilot-Session: 94df0e1d-5dc9-4e71-93d5-d7b522aea46b
Co-authored-by: Copilot <[email protected]> Copilot-Session: 94df0e1d-5dc9-4e71-93d5-d7b522aea46b
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #6767.
Rationale for this change
Spark accepts any
spark.comet.*key, so a typo such asspark.comet.exec.enabeldis 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>.enabledandspark.comet.operator.<Name>.allowIncompatible. There are two ways to validate them:<Name>must be an expression/aggregate with a serde, or the key must be registered (operators)spark.comet.expression.*.enabledetc. is acceptedCastt,WindowExecafter it stopped beingIncompatible)Incompatiblewithout registering its key. The warning is then misleading, but the key still works, becauseisOperatorAllowIncompatreads the value without checking registrationIncompatibleoperators must register their key (documented inadding_a_new_operator.md)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?
CometConfValidator. For each explicitly setspark.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)UTF8String.levenshteinDistance), with ties going to the alphabetically first key. Values are never logged.CometConf.allConfsor from aCometConfigProvider(contrib), including its alternative (deprecated) keys;spark.comet.expression.<Name>.enabled/.allowIncompatiblewhere<Name>is an expression or aggregate that has a serde inQueryPlanSerde;spark.comet.version, which the driver plugin sets itself.allowIncompatiblekeys are only read when an operator serde returnsIncompatible, and today onlyDataWritingCommandExecandWriteFilesExecdo; 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.CometDriverPlugin.initchecks theSparkConf.CometExecRule._applychecks the session conf, before any early return, so a typo that ends up disabling Comet is still reported.spark.comet.operator.WindowExec.allowIncompatibleinCometWindowExecSuite,lag_lead.sqlandCometSlidingSumBenchmark. Window has not returnedIncompatiblesince feat: extend native windows support #4209.spark.comet.exec.shuffle.fallbackToColumnarinCometAggregateSuite.configs.md: describes the warning.adding_a_new_operator.md: an operator that can returnIncompatible()must register itsallowIncompatiblekey withcreateOperatorIncompatConfig(contrib viaCometConfigProvider), otherwise Comet warns about the key.Note: #7567 touches the same spots in
Plugins.scalaandCometExecRule._apply; whichever lands second will need a small rebase.How are these changes tested?
CometConfSuite: suggestions (case, typo, expression name, no close key), registered/alternative/spark.comet.versionkeys, expression and operator keys, and keys that are not checked (spark.comet.datafusion.*, non-Comet keys).CometConfValidatorSuite: warns once across multiple queries, warns even whenspark.comet.enabled=false, and does not warn for recognized keys (log appender based).make format: passed.npx prettier --checkon the two changed docs: passed.Possible follow-ups
These are out of scope for this PR. I would like to know whether they are worth tracking:
Incompatibleoperator has no registered key. With exact checks, an operator (for example in a contrib) that returnsIncompatiblebut does not register itsspark.comet.operator.<Name>.allowIncompatiblekey withcreateOperatorIncompatConfigwould get a misleading warning, although the key still works. TheIncompatiblebranch inCometExecRule(here) could throw when the key is not registered, with a message that explains how to register it. Any test that reaches that path would then fail in CI. The downside is that an out-of-repo contrib that never registered its key would start failing after an upgrade.spark.comet.datafusion.*. This PR passes the whole prefix through. These keys could be checked against DataFusion's known config keys, so typos there are reported too.@andygrove @comphead would you like me to open issues for these?