Implements lowBoundary and highBoundary #229

Diferti · 2024-04-16T13:31:24Z

This pull request implements lowBoundary and highBoundary as functions on userInvocationTable

The FHIRPath spec says that lowBoundary and highBoundary should work with decimal, date, datetime, and time. To come up with idea how keep picoseconds in Javascript was fun.

To implement this, we need to pass a FHIR model to fhirpath.evaluate. If we do not, we will not have TypeInfo to determine dateTime and date because they overlap when using string checking.

I imported R4 model to use in the reference implementation to do this.

Yngwarr

Hello! Great work on implementing boundary functions and adding tests! We'll have to take a closer look at TypeInfo and figure out, why it behaves as it is.

Yngwarr · 2024-04-22T13:07:57Z

sof-js/src/path.js

+
+// @note this is not exported by fhirpath/src/types but should be
+let dateRE = new RegExp(
+  '^[0-9][0-9][0-9][0-9](-[0-9][0-9](-[0-9][0-9])?)?$');


It's better to use FP_Date.checkString for this.

There are also checkString functions for FP_DateTime and FP_Time.

@Yngwarr I have updated the pull request to use a model with fhirpath.evaluate to make this much easier.

Now we get TypeInfo correctly with dateTime, date, and time.

Diferti · 2024-05-03T23:34:36Z

sof-js/src/path.js

@@ -1,4 +1,6 @@
 import { default as fhirpath } from 'fhirpath'
+import fhir_r4_model from 'fhirpath/fhir-context/r4'
+import { FP_DateTime, FP_Time } from 'fhirpath/src/types';


Note: these are not exported, but could be

Diferti · 2024-05-03T23:37:08Z

sof-js/src/path.js

@@ -72,7 +181,7 @@ function process_constants(constants) {
 }

 export function fhirpath_evaluate(data, path, constants = []) {
-  return fhirpath.evaluate(data, rewrite_path(path), process_constants(constants), null, fhirpath_options);
+  return fhirpath.evaluate(data, rewrite_path(path), process_constants(constants), fhir_r4_model, fhirpath_options);


Note: if we don't use model, TypeInfo for Resources will not work.

Question: should we use R4 in reference?

I guess we can. Do we need it to tell date and dateTime apart?

Yes, to determine date and dateTime, it is important to use the model

Yngwarr · 2024-05-07T19:06:18Z

sof-js/src/path.js

+    }
+    if (!day) {
+      if (month == "02") {
+        (year % 4) ? day = '28' : day = '29';


It's a bit more complicated with leap years: years that are divisible by 100 are not leap unless they're also divisible by 400 (see. wikipedia). I'm not sure it's that important in our case, but better safe than sorry.

I know this about leap years. I just thought it is not necessary. Thank you for feedback, i will change condition to handle this type of situation.

Yngwarr · 2024-05-10T10:42:15Z

sof-js/src/path.js

@@ -72,7 +181,7 @@ function process_constants(constants) {
 }

 export function fhirpath_evaluate(data, path, constants = []) {
-  return fhirpath.evaluate(data, rewrite_path(path), process_constants(constants), null, fhirpath_options);
+  return fhirpath.evaluate(data, rewrite_path(path), process_constants(constants), fhir_r4_model, fhirpath_options);


I guess we can. Do we need it to tell date and dateTime apart?

Diferti · 2024-05-16T15:53:58Z

tests/fn_boundary.json

@@ -291,7 +357,7 @@
      "expect": [
        {
          "id": "o1",
-          "decimal": 0.95


If input value is 1.0 then how is it possible to get 0,95? I think it is an error in the test values(same for highBoundary).

Diferti · 2024-05-16T15:57:37Z

tests/fn_boundary.json

+        },
+        {
+          "id": "o26",
+          "decimal": 1.58700000


JS removes all zeros in the end, so this value looks like 1.587, and lowBoundary will return 1.587, although they are both should be 1.58700000.

Diferti · 2024-05-17T19:20:31Z

tests/fn_boundary.json

+        },
+        {
+          "id": "o33",
+          "decimal": 15.0809999


This test failed, because the input value was 15.080. However, JS removes 0 in the end of value, so the input becomes 15.08, and the returned value is 15.08999999, but it should be 15.0809999.

johngrimes

Hi @Diferti, I had a look at your changes, and found a couple of issues relating to the tests being a bit too implementation-specific.

I think if we fix these and merge master back in to this branch, we should be able to finalise this change.

johngrimes · 2024-06-18T23:23:39Z

tests/fn_boundary.json

+        "text": "code"
+      },
+      "status": "final",
+      "valueDateTime": "2010-10-10T15:21"


This is not a valid FHIR dateTime, see: https://hl7.org/fhir/r4/datatypes.html#dateTime

johngrimes · 2024-06-18T23:33:16Z

tests/fn_boundary.json

+      },
+      "status": "final",
+      "valueQuantity": {
+        "value": 123456789123.1234


The result of this test is implementation-specific, what happens if we run this against a runner that can handle higher precision?

Sorry, this is not a test - I mean the tests below that use this resource.

johngrimes · 2024-06-25T03:43:40Z

Digging into this has raised more questions than answers for me - there are multiple versions of the specification and the semantics are not clear to me in any of them.

I've posted a question thread about this in Zulip: https://chat.fhir.org/#narrow/stream/179266-fhirpath/topic/Boundary.20functions

Let's see what the FHIR community comes back with.

johngrimes · 2024-07-16T03:17:21Z

I don't actually think that this is necessary to resolve for the spec to go out - it is more of an issue on the reference implementation.

Implements lowBoundary and highBoundary

74f6bb5

Yngwarr self-assigned this Apr 22, 2024

Yngwarr reviewed Apr 22, 2024

View reviewed changes

Solved issue with determining date and dateTime

611df3c

Diferti commented May 3, 2024

View reviewed changes

Diferti changed the title ~~[WIP] Implements lowBoundary and highBoundary~~ Implements lowBoundary and highBoundary May 3, 2024

Diferti requested a review from Yngwarr May 3, 2024 23:49

Diferti mentioned this pull request May 3, 2024

Decimal for lowBoundary and highBoundary #238

Open

Yngwarr reviewed May 10, 2024

View reviewed changes

Diferti added 2 commits May 10, 2024 14:44

Updated leap year logic, added tests for it

ad02990

Preliminary code for decimal

a81e645

Diferti commented May 16, 2024

View reviewed changes

Fixed 0 precesion for highBoundary

618dc18

Diferti commented May 17, 2024

View reviewed changes

johngrimes requested changes Jun 19, 2024

View reviewed changes

Diferti and others added 3 commits June 19, 2024 21:26

Merge branch 'FHIR:master' into boundary

733c865

Changed tests for review feedback

3f482f0

Merge branch 'boundary' of personal:Diferti/sql-on-fhir-v2 into boundary

e684aa5

Diferti requested a review from johngrimes June 20, 2024 02:09

johngrimes added this to the v2.0.0 milestone Jun 25, 2024

johngrimes assigned johngrimes and unassigned Yngwarr Jun 26, 2024

johngrimes added the enhancement New feature or request label Jun 26, 2024

johngrimes removed this from the v2.0.0 milestone Jul 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements lowBoundary and highBoundary #229

Implements lowBoundary and highBoundary #229

Diferti commented Apr 16, 2024 •

edited

Loading

Yngwarr left a comment

Yngwarr Apr 22, 2024

Diferti May 3, 2024

Diferti May 3, 2024

Diferti May 3, 2024

Yngwarr May 10, 2024

Diferti May 10, 2024

Yngwarr May 7, 2024

Diferti May 10, 2024

Yngwarr May 10, 2024

Diferti May 16, 2024

Diferti May 16, 2024

Diferti May 17, 2024

johngrimes left a comment

johngrimes Jun 18, 2024

johngrimes Jun 18, 2024

johngrimes Jun 19, 2024

johngrimes commented Jun 25, 2024

johngrimes commented Jul 16, 2024

Implements lowBoundary and highBoundary #229

Are you sure you want to change the base?

Implements lowBoundary and highBoundary #229

Conversation

Diferti commented Apr 16, 2024 • edited Loading

Yngwarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngrimes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngrimes commented Jun 25, 2024

johngrimes commented Jul 16, 2024

Diferti commented Apr 16, 2024 •

edited

Loading