issue#108 add support for high and low bondary #109

vfrank66 · 2025-04-19T01:25:46Z

No description provided.

evan-gordon · 2025-04-25T21:27:43Z

interpreter/operator_comparison.go

+	// 1. Determine the precision of each decimal
+	lPrecision := getDecimalPrecision(lDec)
+	rPrecision := getDecimalPrecision(rDec)
+


nit: The following logic could be simplified with the builtin min function https://tip.golang.org/ref/spec#Min_and_max
so minPrecision := min(lPrecision, rPrecision)

Steps 1 & 2 could technically be combined using this method making this all one statement.

evan-gordon · 2025-04-25T21:28:34Z

interpreter/operator_comparison.go

+		precision = rPrecision
+	}
+
+	// 3. Round both numbers to that precision


nit: reword to 'Round both numbers to the minimal common precision.'

evan-gordon · 2025-04-25T22:05:18Z

interpreter/operator_arithmetic.go

+// 1. Ignore the CQL spec, does it really matter?
+// 2. Modify result.Value to hold the original string value, could be perf cost
+// 3. Create custom Decimal type
+func evalPrecisionDecimal(_ model.IUnaryExpression, obj result.Value) (result.Value, error) {


Ive got a few thoughts here.

I think for now I'd lean towards option 1 here for now.

I'm not a fan of needing to rewalk the expression tree inside the interpreter here, the intention of those variables are primarily for debugging / tooling purposes, not really for logic calculations. On top of that this solution isn't really generalizable and can only really handle literals or expressions containing literals.

One question i have related to whether trailing zeroes ever even matter outside of when literal values are passed to this function? So for example for the code

Precision(0.000000 + 0.1)

Imo I don't think so. My intuition would be to expect a result of 1 from this expression.

So maybe if and only if the source expression is a literal we use that? I don't love it but I think in this one case we allow it.

@suyashkumar any thoughts here? I was looking through our old notes and all i could find was that we opted not to implement this at the time because of this edge case. Since some of the logic here needs to be reused for HighBoundary and LowBoundary we should probably pick a path forward here for now.

@evan-gordon so to summarize first check if the source expression is a literal, if true, literal's string value = precision, else, computed values?

meta comment: High-level, how important are HighBoundary and LowBoundary (and Decimal precision) in real world CQL use? I ask because if that's what's forcing this question, we should think about where it falls in the priority list esp if it forces us to have to implement hacks or incorrect logic that may hurt us later

Anyway, it's been a long while so some off the cuff thoughts (take with grain of salt):

Go and CQL uncertainty in floats are fairly different (iiuc). Go floats have a notion of binary bit precision as IEEE‑754 floats, that don't map to CQL base 10 decimal point "precision" though if someone has a link that explains CQL precision better let me know.

What this implies to me is that if we want to correctly represent CQL Decimal precision through operations (math, etc), we'd need to not use Go floats and/or keep track of precision as a side value similar to how we do with Dates/DateTimes etc (here, the actual numbers would be based on Go floats but the precision "math" we'd do seperately). Maybe we could switch to https://pkg.go.dev/math/big#Float but would need to look into the details more (I think we still run into base 2 vs base 10 precision notions?).

For Literals, sure we can get around this by inspecting the source expression string, but this will fail for data pulled in via FHIR, data going through math expressions (unless we go deep through the tree and compute precision through ops, which goes back to what I mentioned earlier where we could handle precision side values like Dates/DateTimes etc).

So ultimately I'd say:

How important are these operators, and in what situations?

Based on that, how much does correctness as per the spec matter in cases beyond literals? Is it useful to implement these but with some well-defined deviations based on the current infra?

Based on that, if near full correctness is important we might as well do this "right" using one of the options above. I don't think this should be a priority though unless driven by real world use cases vs. spending time on other operators!

Like I said, I may have missed something since it's been so long, but just wanted to dump some off the thoughts.

evan-gordon · 2025-04-25T22:09:32Z

interpreter/operator_arithmetic.go

 			return result.Value{}, fmt.Errorf("internal error - evaluation timestamp cannot be nil for DateTime max value")
 		}
-		return result.New(result.DateTime{Date: time.Date(9999, 12, 31, 23, 59, 59, 999, evaluationTimestamp.Location()), Precision: model.MILLISECOND})
+		return result.New(result.DateTime{Date: time.Date(9999, 12, 31, 23, 59, 59, 999000000, time.UTC), Precision: model.MILLISECOND})


This should be reverted (and the associated test as well).
See the comments at the top of the function, the tldr is that this is a case that engines have leeway on, we chose to go this route to make handling some edge cases for datetime evaluation more intuitive / easy to do.

evan-gordon · 2025-04-25T22:11:27Z

interpreter/operator_arithmetic.go

 }

 // Note: For Date/Time based values, the spec states that an engine can choose to set the timezone
 // to UTC for min/max values, we use the evaluation timestamp's timezone.


I believe these comments belong below this new block next to minValue

evan-gordon · 2025-04-25T22:20:49Z

interpreter/operator_arithmetic.go

+
+	var resultStr string
+
+	if precValue > 0 {


I believe based on the discussion at cqframework/cql-tests#44 and the comment linked to there, if the precision value is negative, we should be returning null.

From there we can handle the case where precisionValue == 0 separately and then handle the remaining cases.

evan-gordon · 2025-04-25T22:26:10Z

interpreter/operator_arithmetic.go

+	dateTime := dt.Date
+	year, _, _ := dateTime.Date()
+
+	switch prec {


The spec is a bit vauge at best for how date/datetime should be handled here, imo i think cases like precision of 5 should be handled as if the input was 4 since the spec says "greatest possible value of the input to the specified precision"

evan-gordon · 2025-04-25T22:26:43Z

interpreter/operator_arithmetic.go

+	year, month, day := dateTime.Date()
+	hour, minute, second := dateTime.Clock()
+
+	switch prec {


Similar to my comments for date, also can be applied to the low boundary functions for these types as well.

evan-gordon · 2025-04-25T22:27:42Z

tests/enginetests/operator_arithmetic_test.go

 			name:       "maximum DateTime",
 			cql:        "maximum DateTime",
-			wantResult: newOrFatal(t, result.DateTime{Date: time.Date(9999, 12, 31, 23, 59, 59, 999, defaultEvalTimestamp.Location()), Precision: model.MILLISECOND}),
+			wantResult: newOrFatal(t, result.DateTime{Date: time.Date(9999, 12, 31, 23, 59, 59, 999000000, time.UTC), Precision: model.MILLISECOND}),


As i mentioned above, we should always be testing against the defaultEvalTimestamp.

evan-gordon · 2025-04-25T22:28:46Z

tests/enginetests/operator_arithmetic_test.go

 			cql:        "maximum DateTime",
-			wantResult: newOrFatal(t, result.DateTime{Date: time.Date(9999, 12, 31, 23, 59, 59, 999, defaultEvalTimestamp.Location()), Precision: model.MILLISECOND}),
+			wantResult: newOrFatal(t, result.DateTime{Date: time.Date(9999, 12, 31, 23, 59, 59, 999000000, time.UTC), Precision: model.MILLISECOND}),
 		},


Would you mind adding test cases added to this file for the new operators? :)

suyashkumar

didn't do a full review, just chimed in on the section I was asked!

suyashkumar · 2025-05-02T22:02:57Z

interpreter/operator_arithmetic.go

+// 1. Ignore the CQL spec, does it really matter?
+// 2. Modify result.Value to hold the original string value, could be perf cost
+// 3. Create custom Decimal type
+func evalPrecisionDecimal(_ model.IUnaryExpression, obj result.Value) (result.Value, error) {


meta comment: High-level, how important are HighBoundary and LowBoundary (and Decimal precision) in real world CQL use? I ask because if that's what's forcing this question, we should think about where it falls in the priority list esp if it forces us to have to implement hacks or incorrect logic that may hurt us later

Anyway, it's been a long while so some off the cuff thoughts (take with grain of salt):

Go and CQL uncertainty in floats are fairly different (iiuc). Go floats have a notion of binary bit precision as IEEE‑754 floats, that don't map to CQL base 10 decimal point "precision" though if someone has a link that explains CQL precision better let me know.

What this implies to me is that if we want to correctly represent CQL Decimal precision through operations (math, etc), we'd need to not use Go floats and/or keep track of precision as a side value similar to how we do with Dates/DateTimes etc (here, the actual numbers would be based on Go floats but the precision "math" we'd do seperately). Maybe we could switch to https://pkg.go.dev/math/big#Float but would need to look into the details more (I think we still run into base 2 vs base 10 precision notions?).

For Literals, sure we can get around this by inspecting the source expression string, but this will fail for data pulled in via FHIR, data going through math expressions (unless we go deep through the tree and compute precision through ops, which goes back to what I mentioned earlier where we could handle precision side values like Dates/DateTimes etc).

So ultimately I'd say:

How important are these operators, and in what situations?

Based on that, how much does correctness as per the spec matter in cases beyond literals? Is it useful to implement these but with some well-defined deviations based on the current infra?

Based on that, if near full correctness is important we might as well do this "right" using one of the options above. I don't think this should be a priority though unless driven by real world use cases vs. spending time on other operators!

Like I said, I may have missed something since it's been so long, but just wanted to dump some off the thoughts.

suyashkumar · 2025-05-02T22:03:46Z

interpreter/operator_comparison.go

+func getDecimalPrecision(value float64) int {
+	// Convert to string to determine precision
+	str := strconv.FormatFloat(value, 'f', -1, 64)
+
+	// Find the decimal point
+	decimalPos := strings.IndexRune(str, '.')
+	if decimalPos == -1 {
+		return 0 // No decimal part
+	}
+
+	// Extract the decimal part and trim trailing zeros
+	decimalPart := strings.TrimRight(str[decimalPos+1:], "0")
+	return len(decimalPart)
+}


is this duplicative with logic in evalPrecisionDecimal and can we consolidate?

vfrank66 · 2025-06-03T02:06:10Z

@evan-gordon hey sorry i will get back to this, I have not had time to compleete thee requested PR changes

vfrank66 added 2 commits April 18, 2025 20:25

issue#108 add support for high and low bondary

07442cc

Merge branch 'main' into issue-108/support-highboundary-low-boundary

ec1c20b

evan-gordon reviewed Apr 25, 2025

View reviewed changes

Merge branch 'main' into issue-108/support-highboundary-low-boundary

682ae55

suyashkumar reviewed May 2, 2025

View reviewed changes

Merge branch 'main' into issue-108/support-highboundary-low-boundary

98f6029

issue#108 add support for high and low bondary #109

Are you sure you want to change the base?

issue#108 add support for high and low bondary #109

Uh oh!

Conversation

vfrank66 commented Apr 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suyashkumar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vfrank66 commented Jun 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants