comphead commented on code in PR #4447:
URL: https://github.com/apache/datafusion-comet/pull/4447#discussion_r3312131071
##########
.claude/skills/audit-comet-expression/SKILL.md:
##########
@@ -237,41 +343,172 @@ Also review the Comet implementation (Step 3) against
the Spark behavior (Step 1
- Are there behavioral differences that are NOT marked `Incompatible` but
should be?
- Are there behavioral differences between Spark versions that the Comet
implementation does not account for (missing shim)?
- Does the Rust implementation match the Spark behavior for all edge cases?
-- If `getSupportLevel` returns `Incompatible` or `Unsupported` with a reason,
are `getIncompatibleReasons()` / `getUnsupportedReasons()` also overridden so
the reason is picked up by `GenerateDocs` and appears in the Compatibility
Guide?
+
+### Support-level consistency audit
+
+Walk through this checklist against the serde. Each failed item is a
+finding for Step 6.
+
+1. **Support level matches behavior.** For each branch of
+ `getSupportLevel`, decide whether the user-observable effect is a wrong
+ answer (`Incompatible`) or a fallback / error (`Unsupported`). Flag any
+ branch where the label does not match the behavior.
+2. **Reasons cover every branch.** Every distinct reason that
+ `getSupportLevel` can return as `Incompatible(Some(r))` must appear in
+ `getIncompatibleReasons()`. Same for `Unsupported(Some(r))` and
+ `getUnsupportedReasons()`. Missing reasons silently drop from the
+ Compatibility Guide.
+3. **Reasons are not dead code.** If `getIncompatibleReasons()` is
+ overridden but `getSupportLevel` never returns `Incompatible(...)`,
+ either the reason is stale or `getSupportLevel` is missing a branch.
+ Same for `getUnsupportedReasons()`.
+4. **Reason strings are shared via `private val`.** If the same reason
+ appears as a string literal in two places, flag it: changes to one
+ will not propagate to the other.
+5. **Inline reason matches the constant.** If a `private val reason` is
+ declared but `getSupportLevel` uses a different string literal, flag
+ it as a drift bug.
+6. **`Incompatible(None)` has no docs-only reason.** If
+ `getSupportLevel` returns `Incompatible(None)` but
+ `getIncompatibleReasons()` is non-empty, flag it: the EXPLAIN message
+ will be generic while the docs show a specific reason. Switch to
+ `Incompatible(Some(reason))`.
+7. **`convert` fallbacks are documented.** If `convert` calls
+ `withInfo(expr, "...")` and returns `None` for cases not covered by
+ `getSupportLevel` (e.g. non-literal arguments, unsupported expression
+ shapes), confirm the reason is also listed in
+ `getUnsupportedReasons()`.
+8. **Compatibility decisions live in `getSupportLevel`.** If `convert`
+ reads a config flag and bails out, prefer moving the check into
+ `getSupportLevel` so the dispatcher handles `allowIncompatible`
+ uniformly. `CometCaseConversionBase` is an example of the in-`convert`
+ pattern that this skill recommends against.
+9. **Reason wording.** Each reason should describe the user-observable
+ effect, use sentence case with a period, backtick config keys and
+ types, and link a tracking issue when one exists. Flag reasons that
+ read like internal implementation notes ("DataFusion probes the longer
+ side") or that mismatch their support level (an "Incompatible" reason
+ that says "X is not supported").
---
## Step 6: Recommendations
Summarize findings as a prioritized list.
-### High priority
+### High priority: correctness divergences
-Issues where Comet may silently produce wrong results compared to Spark.
+Cases where Comet produces a different observable result from Spark
+(wrong value, missing exception, accepted-instead-of-rejected input,
+etc.). Each one becomes a captured test in Step 7.
-### Medium priority
+### Medium priority: missing test coverage
Review Comment:
this prob should be a `high priority`? as missing tests can lead to
correctness divergence
--
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]
