-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
613 Fix CqlComparer for tuples #614
base: develop-2.0
Are you sure you want to change the base?
613 Fix CqlComparer for tuples #614
Conversation
…mpiler' into 613-fix-integration-test---systemargumentexception-cannot-generate-a-hash-code-for-valuetuple3-arg_paramname_name
…mpiler' into 613-fix-integration-test---systemargumentexception-cannot-generate-a-hash-code-for-valuetuple3-arg_paramname_name
…m to work in Release build
…mpiler' into 613-fix-integration-test---systemargumentexception-cannot-generate-a-hash-code-for-valuetuple3-arg_paramname_name
{ | ||
0 => Comparer<TValue>.Default.Compare(x.Value, y.Value), | ||
0 => cqlComparer.Compare(x.Value, y.Value, precision), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this a bug, the fact that precision wasn't passed on to the compare?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .NET Comparer wasn't capable of taking a precision. This was replaced with the ICqlComparer, it would make sense to use precision on KeyValuePair's Value property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have a few questions to double check. Could the change made by passing on precision create regressions for HEDIS?
Fix for
Work
TupleComparer
withCqlTupleTypeComparer
. The new comparer compares value tuples where the first parameter is of typeCqlTupleMetadata
and uses the metadata to compare items.TupleBaseTypeComparer
cannot be removed yet, since some unit tests still execute against directly compiled LambdaExpressions, instead of going through the C# code writer and assembly compiled.CqlTupleMetadata
had to be moved to a more base project and was moved fromHl7.Cql.Runtime' to
Hl7.Cql.Primitives`TypeExtensions
had to move to a more base project too, and was moved fromHl7.Cql.Compiler
toHl7.Cql.Primitives
. The extensions for this particularTypeExtensions
is for types related to CQL.TypeExtensionsTests
Integration Runner
While working on this PR, I also looked at the integration tests for CMS measure. I found they were flaky, due to the libraries sometimes failing to create. This was due to the CqlTupleMetadata calculating the hashcode in the constructor, and that used Hasher.Instance which uses MD5 to hash a string. For some strange reason, a NullReferenceException is sometimes thrown deep inside the stack trace. I changed the
CqlTupleMetadata
to do hash calculation lazily.I also discovered that the
KeyValuePairComparer<TKey,TValue>
comparer fails whenTValue
is anobject
when doing a compare. An error is thrown stating that 'object' is not comparable. I fixed this by using the ICqlComparer on key and value instead.Test pass rate increased from 68% to 72%