Skip to content

Commit 92adb14

Browse files
committed
docs: address audit-skill failure modes surfaced by PR apache#4461
The string-expressions audit (PR apache#4461) revealed six recurring failure modes where the skill documented findings rather than acting on them. Strengthen the consistency checklist and auto-fix list to close these loopholes: - Add checklist item 10: expression-shape restrictions (literal-only argument, child data type, etc.) must be declared in `getSupportLevel`, not gated inside `convert` with `withInfo`. Cite `CometLeft` / `CometRight` / `CometSubstring` as the canonical example. - Add checklist item 11: Spark 4.0+ collation routing through `CollationSupport.X.exec` and `StringTypeWithCollation` means the expression is `Incompatible` for non-default collations. Link apache#4496 as the umbrella issue and reject "behaviour unchanged for `UTF8_BINARY`" as a justification for `Compatible`. - Add checklist item 12: a sub-bullet that says "Known divergence" or "Known limitation" on a `Compatible` branch is a smell. The skill must promote the support level rather than documenting the divergence in prose only. Cite the `replace` empty-search-string case. - Add checklist item 13: unreachable serde registrations (e.g. the `btrim` mapping for `StringTrimBoth`, which is rewritten by `RuntimeReplaceable` before serde runs) must be deleted, not catalogued. - Add an issue-verification step to the reason-wording guidance and the follow-up-issue workflow. Every cited issue must be opened with `gh issue view` to confirm it exists, is open, and matches the divergence before the URL ships in a reason string or support-doc sub-bullet. Add the matching auto-fix patterns to Step 7's "apply fixes automatically" list so future audits resolve these inline rather than filing them as prose follow-ups.
1 parent d518e6b commit 92adb14

1 file changed

Lines changed: 73 additions & 1 deletion

File tree

  • .claude/skills/audit-comet-expression

.claude/skills/audit-comet-expression/SKILL.md

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,11 @@ and in EXPLAIN output, so they are user-facing.
209209
- Use backticks around config keys, type names, and SQL identifiers.
210210
- Link to a tracking GitHub issue for known incompatibilities so users can
211211
follow progress: `(https://github.com/apache/datafusion-comet/issues/NNNN)`.
212+
**Verify the issue exists and is open** before citing it
213+
(`gh issue view <N> --repo apache/datafusion-comet`). Issue numbers
214+
invented from context or recalled from memory are a recurring failure
215+
mode: a stale link is worse than no link because the reader follows it
216+
and finds nothing.
212217
- Keep it concise. Single sentence is best.
213218
- Do not write "Incompatible reason: ..." or "Unsupported because ...".
214219
The doc generator adds the framing.
@@ -390,6 +395,49 @@ finding for Step 6.
390395
read like internal implementation notes ("DataFusion probes the longer
391396
side") or that mismatch their support level (an "Incompatible" reason
392397
that says "X is not supported").
398+
10. **Expression-shape restrictions live in `getSupportLevel`.** Any
399+
restriction that is knowable from the expression alone (literal-only
400+
arguments, unsupported child data type, foldable-only options, a
401+
specific operator shape) must be declared as an
402+
`Unsupported(Some(reason))` branch in `getSupportLevel`, not gated
403+
inside `convert` with a `withInfo` + `return None`. Putting the
404+
check in `convert` means EXPLAIN surfaces the reason only at
405+
conversion time, the doc generator never sees it, and the
406+
dispatcher cannot route around it. The literal-only `len`
407+
restrictions on `CometLeft`, `CometRight`, and `CometSubstring`
408+
are the canonical example of the in-`convert` pattern that this
409+
skill forbids: lift them into `getSupportLevel`.
410+
11. **Spark 4.0 collation divergences are flagged, not glossed over.**
411+
If the Spark 4.0/4.1 implementation routes through
412+
`CollationSupport.X.exec(..., collationId)` (or uses
413+
`StringTypeWithCollation` / `StringTypeNonCSAICollation` for input
414+
types) and the Comet path does not propagate collation, the
415+
expression is `Incompatible` for non-default collations. Mark the
416+
branch `Incompatible(Some(reason))` linking to the collation
417+
umbrella issue
418+
(https://github.com/apache/datafusion-comet/issues/4496) so the
419+
follow-up sweep can find every site. "Behaviour unchanged for
420+
`UTF8_BINARY`" alone is not a justification for leaving the
421+
support level at `Compatible`: users running with non-default
422+
collations get silently wrong answers.
423+
12. **Known divergences flip the support level.** If you find yourself
424+
writing the words "Known divergence" or "Known limitation" in the
425+
support-doc sub-bullet while leaving `getSupportLevel` returning
426+
`Compatible`, the audit has skipped its job. A documented
427+
divergence is by definition not `Compatible`. Promote the branch
428+
to `Incompatible(Some(reason))` (or `Unsupported` if the native
429+
path errors rather than producing a wrong answer) and link the
430+
tracking issue. The `replace` empty-search-string divergence with
431+
DataFusion is the canonical example of this anti-pattern.
432+
13. **Unreachable serde mappings are removed.** Expressions registered
433+
as `RuntimeReplaceable` (or otherwise rewritten by an analyzer
434+
rule before serde) never reach `QueryPlanSerde.exprToProtoInternal`
435+
with their original class. If the audit finds that a registered
436+
`CometScalarFunction("name")` or `CometExpressionSerde` entry can
437+
never be hit (e.g. the `btrim` mapping for `StringTrimBoth`, which
438+
is rewritten to `StringTrim` before serde runs), delete the
439+
registration in the same audit PR. Documenting the dead code in
440+
the support doc is not enough.
393441

394442
---
395443

@@ -509,6 +557,24 @@ need user approval. The classes of fix are:
509557
- Hoist a reason shared by multiple serdes (e.g. a recurring
510558
TimestampNTZ caveat) into a small `private object` companion in the
511559
same file, mirroring `UTCTimestampSerde`.
560+
- Lift expression-shape restrictions (literal-only argument, foldable
561+
child, unsupported child data type) out of `convert`'s `withInfo` +
562+
`return None` and into an `Unsupported(Some(reason))` branch in
563+
`getSupportLevel`. The `convert` body should then assume the
564+
precondition holds and stop calling `withInfo` for that case.
565+
- Promote a documented "Known divergence" or "Known limitation" sub-
566+
bullet from a `Compatible` branch to `Incompatible(Some(reason))`
567+
(or `Unsupported` if the native path errors) and link the tracking
568+
issue. The sub-bullet stays as user-facing documentation. The
569+
support level catches up to match.
570+
- Mark expressions whose Spark 4.0+ path routes through
571+
`CollationSupport.X.exec` (or accepts `StringTypeWithCollation` /
572+
`StringTypeNonCSAICollation`) as `Incompatible(Some(reason))` for
573+
non-default collations, linking
574+
https://github.com/apache/datafusion-comet/issues/4496.
575+
- Delete unreachable serde registrations (`RuntimeReplaceable` rewrites
576+
the expression before serde runs, an analyzer rule strips the case,
577+
etc.) rather than documenting them as a curiosity.
512578

513579
Each fix is one of these patterns. If a finding requires a semantics
514580
decision (e.g. is a specific branch really `Unsupported`, or is it
@@ -542,7 +608,13 @@ For each follow-up:
542608

543609
1. Search for an existing issue first
544610
(`gh issue list --search "<expression> <symptom> in:title,body" --state all --limit 5`).
545-
If one exists, link it from the PR description and the support-doc
611+
If a candidate match comes back, **open it
612+
(`gh issue view <N> --repo apache/datafusion-comet`) and confirm the
613+
title and body actually describe the divergence you found, and that
614+
the issue is still `OPEN`**. A closed-but-fixed issue cited as
615+
"known divergence" is worse than no citation, because the reader
616+
follows the link and finds a fix that was already shipped. If it
617+
matches, link it from the PR description and the support-doc
546618
sub-bullet, and stop.
547619
2. If no issue exists, file one with `gh issue create` using the
548620
`correctness` label (or `documentation` for doc-only gaps) plus any

0 commit comments

Comments
 (0)