Implement missing features from the latest version of the fhirpath spec

[brianpos]

Implemented the following features:

• Updated to latest fhirpath.g4 grammar
• Long datatype parsing/processing
• lastindexof()
• coalesce()
• sort()
• matchesfull(), and the new optional matches flags parameter
• pathname()
• yearOf, monthOf, dayOf, hourOf, minuteOf, secondOf, millisecondOf, timezoneOffsetOf, dateOf, timeOf
• escape/unescape

[plynchnlm]

surely there must be a better way to handle an arbitrary number of arguments?

[brianpos]

There was in the Firely SDK, but that was a bigger scope than I was happy to make.
I'm happy to give that a whirl, but I thought this was a pragmatic answer that would work for "most" uses...
(same with Sort)

[plynchnlm]

Please go ahead. I think we need a new syntax here to express "1 or more of these things". Maybe we could borrow the + from regular expressions, and write:
coalesce: {fn: filtering.coalesce, arity: {1: ["ExprAtCurrent+"]}
Something with a + would need to be the last argument.

[plynchnlm]

Or:
coalesce: {fn: filtering.coalesce, arity: {*: ["ExprAtCurrent+"]}

[plynchnlm]

No, that needs a + too:

coalesce: {fn: filtering.coalesce, arity: {+: ["ExprAtCurrent+"]}

[brianpos]

I went a slightly different direction:

  coalesce:     {fn: filtering.coalesce, params: {argType: "ExprAtCurrent", minArgCount: 1 }},
  sort:         {fn: filtering.sort, params: {argType: "SortArgument"}},

And then updated the doInvoke to handle it.
This is specific to only sort and coalesce so think its fine.
(sent screenshot of change to doInvoke on Zulip)

[plynchnlm]

How about a test for an inappropriate data type, like 5.yearOf()?

[brianpos]

Great find, this is also a gap in the spec!
https://jira.hl7.org/browse/FHIR-55946

[yuriy-sedinkin]

Similar actions are performed in compareValues->typecheck->util.assertOnlyOne later in this function

[brianpos]

My manual checking complains earlier I guess (and as a result gets a more specific error message)
I still call the type check to ensure that the types are valid though right?

[yuriy-sedinkin]

Performance is critical for evaluating FHIRPath expressions. If we can avoid repetition, we should avoid it.

[yuriy-sedinkin]

Similar actions are performed in compareValues->typecheck->util.valDataConverted later in this function.

Perhaps this block from // Enforce singleton requirement per specification to // Compare values using FHIRPath comparison semantics could be removed, and the compareValues function modified slightly?

[brianpos]

the usage in the compare values is kinda different to this part here, as in the compare values, that's handling the considerations of ordering where a value is null or not, where this part is determining if each value is null, and making that consistent. And then being able to flow down to the next sort parameter if required.
Null doesn't mean stop, it can mean look at the next parameter...

[yuriy-sedinkin]

The tests pass if I replace those two functions with the following code (it is shorter, but I guess it could still be improved for better performance):

engine.sort = function(data, ...sortArgs) {
  if(data !== false && !data) { return []; }
  
  // If no sort arguments provided, use natural ordering
  if (sortArgs.length === 0) {
    return data.slice().sort((a, b) => {
      return compareValues([a], [b]);
    });
  }
  
  // Sort with expressions and directions
  return data.slice().sort((a, b) => {
    for (let i = 0; i < sortArgs.length; i++) {
      const sortArg = sortArgs[i];
      const direction = sortArg.direction || 'asc';
      
      // Evaluate the sort expression for both items
      const resultA = sortArg.expr([a]);
      const resultB = sortArg.expr([b]);

      // Compare values using FHIRPath comparison semantics
      let comparison = compareValues(resultA, resultB);
      
      // Apply direction
      if (direction === 'desc') {
        comparison = -comparison;
      }
      
      // If this sort key produces a difference, return it
      if (comparison !== 0) {
        return comparison;
      }
      
      // Otherwise, continue to the next sort key
    }
    
    // All sort keys were equal
    return 0;
  });
};

/**
 * Compare two values using FHIRPath comparison semantics
 * Reuses existing equality.js comparison logic
 */
function compareValues(a, b) {
  // Use existing FHIRPath comparison logic from equality.js
  // Convert to singleton arrays for typecheck
  const [a0, b0] = equality.typecheck(a, b);

  // Handle empty values - per spec: "An empty value is considered lower than all other values"
  if (a0 == null && b0 == null) return 0;
  if (a0 == null) return -1;  // Empty values sort before non-empty values
  if (b0 == null) return 1;

  // Handle FP_Type objects (dates, times, quantities, etc.)
  if (a0 instanceof FP_Type) {
    const compareResult = a0.compare(b0);
    return compareResult === null ? 0 : compareResult;
  }
  
  // TODO: Tests passed without error if I comment out the following code:
  // // Reject non-primitive types that can't be meaningfully compared
  // // Per spec: "Values that would result in comparison errors must be filtered prior to sorting"
  // let type = typeof a0;
  // if (type === 'object' || type === 'function') {
  //   throw new Error('Cannot sort by non-primitive type: ' + (a0.constructor?.name || type));
  // }
  // type = typeof b0;
  // if (type === 'object' || type === 'function') {
  //   throw new Error('Cannot sort by non-primitive type: ' + (b0.constructor?.name || type));
  // }
  
  // Standard JavaScript comparison for basic types
  if (a0 === b0) return 0;
  return a0 < b0 ? -1 : a0 > b0 ? 1 : 0;
}

Do you think these changes look good?
If you can improve these changes, that would be great.
Otherwise, I will think about it before merging this pull request.

I commented out the block of code about "Reject non-primitive types that can't be meaningfully compared" and all tests passed. An additional unit test is needed for this block of code.

[yuriy-sedinkin]

The tests pass if I replace those two functions with the following code (it is shorter, but I guess it could still be improved for better performance):

engine.sort = function(data, ...sortArgs) {
  if(data !== false && !data) { return []; }
  
  // If no sort arguments provided, use natural ordering
  if (sortArgs.length === 0) {
    return data.slice().sort((a, b) => {
      return compareValues([a], [b]);
    });
  }
  
  // Sort with expressions and directions
  return data.slice().sort((a, b) => {
    for (let i = 0; i < sortArgs.length; i++) {
      const sortArg = sortArgs[i];
      const direction = sortArg.direction || 'asc';
      
      // Evaluate the sort expression for both items
      const resultA = sortArg.expr([a]);
      const resultB = sortArg.expr([b]);

      // Compare values using FHIRPath comparison semantics
      let comparison = compareValues(resultA, resultB);
      
      // Apply direction
      if (direction === 'desc') {
        comparison = -comparison;
      }
      
      // If this sort key produces a difference, return it
      if (comparison !== 0) {
        return comparison;
      }
      
      // Otherwise, continue to the next sort key
    }
    
    // All sort keys were equal
    return 0;
  });
};

/**
 * Compare two values using FHIRPath comparison semantics
 * Reuses existing equality.js comparison logic
 */
function compareValues(a, b) {
  // Use existing FHIRPath comparison logic from equality.js
  // Convert to singleton arrays for typecheck
  const [a0, b0] = equality.typecheck(a, b);

  // Handle empty values - per spec: "An empty value is considered lower than all other values"
  if (a0 == null && b0 == null) return 0;
  if (a0 == null) return -1;  // Empty values sort before non-empty values
  if (b0 == null) return 1;

  // Handle FP_Type objects (dates, times, quantities, etc.)
  if (a0 instanceof FP_Type) {
    const compareResult = a0.compare(b0);
    return compareResult === null ? 0 : compareResult;
  }
  
  // TODO: Tests passed without error if I comment out the following code:
  // // Reject non-primitive types that can't be meaningfully compared
  // // Per spec: "Values that would result in comparison errors must be filtered prior to sorting"
  // let type = typeof a0;
  // if (type === 'object' || type === 'function') {
  //   throw new Error('Cannot sort by non-primitive type: ' + (a0.constructor?.name || type));
  // }
  // type = typeof b0;
  // if (type === 'object' || type === 'function') {
  //   throw new Error('Cannot sort by non-primitive type: ' + (b0.constructor?.name || type));
  // }
  
  // Standard JavaScript comparison for basic types
  if (a0 === b0) return 0;
  return a0 < b0 ? -1 : a0 > b0 ? 1 : 0;
}

Do you think these changes look good?
If you can improve these changes, that would be great.
Otherwise, I will think about it before merging this pull request.

I commented out the block of code about "Reject non-primitive types that can't be meaningfully compared" and all tests passed. An additional unit test is needed for this block of code.