andygrove opened a new pull request, #4447:
URL: https://github.com/apache/datafusion-comet/pull/4447
## Which issue does this PR close?
Closes #.
## Rationale for this change
The `audit-comet-expression` skill produced inconsistent guidance on when to
use `Unsupported` vs `Incompatible`, and how `getSupportLevel`,
`getIncompatibleReasons`, `getUnsupportedReasons`, and `convert` need to stay
aligned. As a result, audits flagged some real issues but missed others, and
the recommended fixes were vague.
Reviewing the existing serdes shows several recurring bugs that the skill
should reliably catch:
- `CometHour` declares `private val incompatReason` then hardcodes a
near-duplicate string in `getSupportLevel`, so the EXPLAIN message and the
Compatibility Guide can drift.
- `CometMinute` and `CometSecond` duplicate the same reason text in two
places with no shared constant.
- `CometInitCap` returns `Incompatible(None)` while
`getIncompatibleReasons()` is populated, so the reason appears in docs but not
in EXPLAIN output.
- Some `Incompatible` reasons read like "X is not supported", which is
really an `Unsupported` reason in disguise.
## What changes are included in this PR?
Updates to `.claude/skills/audit-comet-expression/SKILL.md`:
- **Step 3** gains a dedicated "Audit `getSupportLevel`,
`getIncompatibleReasons`, `getUnsupportedReasons`, and `convert`" subsection
covering:
- Decision rule for `Unsupported` vs `Incompatible` (wrong answer vs
fallback/error).
- Runtime vs docs split: support-level `notes` flow into EXPLAIN via the
dispatcher in `QueryPlanSerde.exprToProtoInternal`; `get*Reasons()` is read
only by `GenerateDocs`.
- Six consistency rules (cover every branch, no dead reasons, share via
`private val`, prefer `Some(reason)` over `None`, gate decisions in
`getSupportLevel` rather than `convert`).
- Wording guidelines for reason strings (user-observable effect first,
sentence case, backtick configs/types, link tracking issues).
- Real antipattern callouts naming specific serdes.
- **Step 5** gains a structured 9-item support-level consistency audit
checklist that produces findings against the four methods.
- **Step 7** splits into two asks: one for missing tests, one for
consistency fixes, so the user can opt in to either independently.
## How are these changes tested?
This change is documentation for a Claude skill and has no code or test
impact. Verified by re-reading the skill end-to-end against several real serdes
(`CometArrayIntersect`, `CometHour`, `CometInitCap`, `CometSortArray`,
`CometStructsToJson`) to confirm the new guidance correctly identifies their
alignment state.
--
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]
