Quantities and date arithmetic

[johngrimes]

Resolves #340 and #341.

[codecov]

Codecov Report

Base: 85.52% // Head: 86.06% // Increases project coverage by +0.54% 🎉

Coverage data is based on head (f12660d) compared to base (c9550ba).
Patch coverage: 89.02% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #509      +/-   ##
============================================
+ Coverage     85.52%   86.06%   +0.54%     
  Complexity       25       25              
============================================
  Files           219      254      +35     
  Lines          5809     6338     +529     
  Branches        419      449      +30     
============================================
+ Hits           4968     5455     +487     
- Misses          622      639      +17     
- Partials        219      244      +25     

Impacted Files	Coverage Δ
...au/csiro/pathling/config/StorageConfiguration.java	66.66% <ø> (-11.12%)	⬇️
...main/java/au/csiro/pathling/fhirpath/FhirPath.java	0.00% <0.00%> (ø)
.../main/java/au/csiro/pathling/fhirpath/Numeric.java	100.00% <ø> (ø)
...ain/java/au/csiro/pathling/sql/SqlExpressions.java	100.00% <ø> (ø)
...au/csiro/pathling/sql/dates/time/TimeFunction.java	0.00% <0.00%> (ø)
...csiro/pathling/encoders/terminology/ucum/Ucum.java	44.44% <44.44%> (ø)
...athling/sql/dates/time/TimeComparisonFunction.java	50.00% <50.00%> (ø)
.../pathling/fhirpath/literal/BooleanLiteralPath.java	90.00% <66.66%> (-1.67%)	⬇️
...ro/pathling/sql/dates/time/TimeEqualsFunction.java	66.66% <66.66%> (ø)
...thling/sql/dates/time/TimeGreaterThanFunction.java	66.66% <66.66%> (ø)
... and 68 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[johngrimes]

I think there is some missing test coverage relating to the use of Integer or Decimal types with a Quantity and a math operator. Currently an expression such as 1 * 1 'm' causes an error. It passes validation as both paths are now Numeric.

I will add some more tests to cover this, and make the appropriate adjustments to the implementation.

[johngrimes]

In cases where one operand is a Quantity and the other is an Integer or Decimal, I think the correct solution to this is to convert the Integer or Decimal to a Quantity with a unit of '1'.

In the Operators > Math section, the spec says:

Both operands must be of the same type, or of compatible types according to the rules for implicit conversion.

The conversion table says that Integer and Decimal types can be implicitly converted to Quantity.

[johngrimes]

After a brief trip down that rabbit hole, I worked out that implementing that part of the specification actually provides very little value.

I have resolved this problem by adding a new test: MathOperatorValidationTest#operandsAreNotComparable.

I have also added additional validation to MathOperator to check that the operands are comparable.

[johngrimes]

@piotrszul There seem to be two classes, FlexiDecimal and FlexDecimal.

FlexDecimal seems to only be used within the decimal benchmark, I am going to assume that this is a mistake and that we should be using FlexiDecimal everywhere...

[johngrimes]

Or were these two different implementations that you were comparing?

Can we retire one of them now?

[piotrszul]

After a brief trip down that rabbit hole, I worked out that implementing that part of the specification actually provides very little value.

I have resolved this problem by adding a new test: MathOperatorValidationTest#operandsAreNotComparable.

I have also added additional validation to MathOperator to check that the operands are comparable.

Perhaps this can be addressed (the implicit conversions) when (if ever) we implement the explicit conversion functions (e.g.: toQuantity()). Because then it would be just the matter of 'implicitly' applying this function in certain situations.

[johngrimes]

Perhaps this can be addressed (the implicit conversions) when (if ever) we implement the explicit conversion functions (e.g.: toQuantity()). Because then it would be just the matter of 'implicitly' applying this function in certain situations.

Yes, I have a good idea of how it could be done - I'm just not sure that we have a solid use case for it at the moment.

[piotrszul]

Or were these two different implementations that you were comparing?

Can we retire one of them now?

Yes, we can retire the string based implementation (FlexDecimal). As for the benchmark I would prefer to retain it (for decimal and FlexiDecimal) but just do not include it in the build. It may still be useful to run it locally to possibly further optimize FlexiDeciamal performance. The easies way to achieve it is to rename the class so that it does not fit the patters from PathlingBenchmarkRunner (.include("\.[^.]+Benchmark\.[^.]+$")).
Or maybe move such local benchmark to a different package and adjust the patter above.

[johngrimes]

Ok, I have kept the DecimalBenchmark in but disabled, and with FlexDecimal removed.