Pathling Fhirpath Review

Pathling FHIRPath review

A correctness rubric for FHIRPath implementation changes. It exists so a reviewer checks the things that actually break in this codebase, rather than producing generic code-review output.

Review is most useful in a fresh context that sees the diff and this rubric but not the reasoning that produced the change. When invoked as part of a larger workflow, dispatch a subagent with the diff range and this rubric rather than reviewing inline.

Establish the range

BASE_SHA=$(git merge-base origin/main HEAD)
git diff --stat $BASE_SHA..HEAD
git diff $BASE_SHA..HEAD

Read the linked issue and the governing spec section before judging behaviour. Use the fhirpath-spec skill for spec lookups — the spec decides, not intuition.

When this rubric is being applied inside a dispatched subagent — the usual case, and what the paragraph above recommends — there is no user to answer a question, so pass --unattended to fhirpath-spec. Without it, a missing reference-implementation pin leaves that skill waiting on an answer that cannot arrive.

Rubric

Work through these in order. The first five are where FHIRPath implementations actually go wrong.

1. Collection semantics

Every FHIRPath expression evaluates to a collection. Check each of the three input shapes is handled as the spec requires:

• Empty — does {} propagate, or does the spec require a value (count() → 0, empty() → true)? Empty-propagation-by-default is right for most functions and wrong for existence and aggregate functions.
• Singleton — the common case, usually correct.
• Multi-item — where a function expects a single item, the spec's singleton evaluation rules apply: one item is used, empty returns empty, and multiple items are an error. Silently taking the first item is a bug.

2. Column cardinality

In the Spark layer a singular FHIR element is a scalar column and a non-singular element is an array column. Code that works on one can fail on the other. Confirm the change handles both, and that tests cover both — a test suite using only literals exercises neither.

3. Null versus empty

Spark nulls and FHIRPath's empty collection are related but not identical. Check that a null column value produces empty rather than propagating a null into a result struct, and that a typed-empty field behaves the same as an absent one where the spec says it should.

4. Type coercion and promotion

• Integer to Decimal promotion in mixed arithmetic and comparison
• String conversions that must return empty rather than throw on unparseable input
• Date/time partial precision: differing precision alone does not make a comparison empty. Components are compared in order, and a difference found before the coarser operand runs out of precision gives a definite answer — @2018-03-01 > @2018-01-01 is true, @2012-01 = @2013 is false. The result is empty only when the precisions differ and the values are equal up to the coarser one, as in @2018-03 > @2018-03-01 or @2012-01 = @2012 — equal values at matching precision are simply equal, so @2012 = @2012 is true. Applies to = as well as </>; both directions of this have caused regressions before
• Quantity units — calendar durations and UCUM units are not interchangeable above seconds

5. Error versus empty

The spec distinguishes "return empty" from "signal an error", and they are easy to conflate. toInteger() on a non-numeric string returns empty; single() on a multi-item collection errors. Check each failure path against the spec text, and check the tests assert the right one — testEmpty and testError are not interchangeable.

6. Spec fidelity

• Behaviour matches the spec section, including its examples
• Javadoc carries an @see link to the governing spec section, as the existing providers do
• Any deliberate divergence is called out in a comment with its reason, not left silent

7. Registration and annotations

• @FhirPathFunction on the method; a new provider class also needs a MethodDefinedFunction.mapOf line in StaticFunctionRegistry
• @SqlOnFhirConformance(Profile.…) where the function maps to a SQL-on-FHIR profile feature — check against sibling functions rather than guessing
• @Nonnull / @Nullable on parameters, returns, and fields
• Substantial logic lives in a *Logic helper (as ConversionFunctions delegates to ConversionLogic), not inline in the provider method