Remove +1 assumption for input versions in $import #3685

SergeyGaluzo · 2024-01-23T19:24:29Z

There is demand to honor arbitrary non sequential resource versions provided for $import. Currently versions are assumed to be sequential ints. This PR enables any int version.

abiisnn · 2024-01-23T23:58:37Z

src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs

@@ -164,12 +164,12 @@ internal class SqlServerFhirDataStore : IFhirDataStore, IProvideCapability
            (var transactionId, var minSequenceId) = await StoreClient.MergeResourcesBeginTransactionAsync(resources.Count, cancellationToken);

            var index = 0;
-            var mergeWrappers = new List<MergeResourceWrapper>();
+            var mergeWrappersPlus = new List<(MergeResourceWrapper Wrapper, bool KeepVersion, int ResourceVersion, int? ExistingVersion)>();


Can we use the same name than before? "plus" without context could lead confusion, perhaps, adding a comment? #Closed

Plus indicates that this variable contains not only merge wrappers but other data points too. There are 3 other data points.

mergeWrappersAndVersions :)

test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs

brendankowitz · 2024-01-25T17:45:32Z

src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs

+            // In each group find the smallest version higher then existing
+            prevResourceId = string.Empty;
+            var notSetInResoureGroup = false;
+            foreach (var mergeWrapperPlus in mergeWrappersPlus.Where(_ => _.KeepVersion && _.ExistingVersion != 0).OrderBy(_ => _.Wrapper.ResourceWrapper.ResourceId).ThenBy(_ => _.ResourceVersion))


nit: _ as a variable confuses tooling, x is just as short ;) #Closed

:-) I like _ as it denotes that it is derived from the definition of enumerable, nothing else. In complex cases when there is more than one thing derived, I use meaningful variable names to differentiate between cases. BTW All code scanner are happy...

Using other variable names gives the more IntelliSense:

When hovering over _, my VS shows correct type details.

This one is also documented in the C# fundamentals: https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/functional/discards. I'd suggest that this would break existing assumptions that someone might have when reading this code.

Replaced _ by x.
BTW In this article they are talking about treating _ as discards in the outputs of some methods. I don't see _ used inside LINQ expression as an output. So, I don't see a violation here.

brendankowitz · 2024-01-25T17:56:12Z

src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs

-            if (mergeWrappers.Count > 0) // Do not call DB with empty input
+            // Resources with input versions (keepVersion=true) might not have hasVersionToCompare set. Fix it here.
+            // Resources with keepVersion=true must be in separate call, and not mixed with keepVersion=false ones.
+            // Sort them in groups by resource id and order by version.


Could this be done with Linq to simplify? Something like:

mergeWrappersPlus .Where(x => x.KeepVersion && x.ExistingVersion != 0) .GroupBy(key => key.Wrapper.ResourceWrapper.ResourceId, items => items.OrderBy(y =>y.ResourceVersion)) ``` #ByDesign

I was not able to find a clear way to apply required logic only using Linq. So I did what I did. I hope it is obvious, hence simple.

brendankowitz · 2024-01-25T18:01:50Z

src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs

@@ -164,12 +164,12 @@ internal class SqlServerFhirDataStore : IFhirDataStore, IProvideCapability
            (var transactionId, var minSequenceId) = await StoreClient.MergeResourcesBeginTransactionAsync(resources.Count, cancellationToken);

            var index = 0;
-            var mergeWrappers = new List<MergeResourceWrapper>();
+            var mergeWrappersPlus = new List<(MergeResourceWrapper Wrapper, bool KeepVersion, int ResourceVersion, int? ExistingVersion)>();


mergeWrappersAndVersions :)

brendankowitz · 2024-01-26T20:12:26Z

src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs

+            prevResourceId = string.Empty;
+            var notSetInResoureGroup = false;
+            foreach (var mergeWrapper in mergeWrappersWithVersions.Where(_ => _.KeepVersion && _.ExistingVersion != 0).OrderBy(_ => _.Wrapper.ResourceWrapper.ResourceId).ThenBy(_ => _.ResourceVersion))
+            {
+                if (prevResourceId != mergeWrapper.Wrapper.ResourceWrapper.ResourceId) // this should reset flag on each resource id group including first.
+                {
+                    notSetInResoureGroup = true;
+                }
+
+                prevResourceId = mergeWrapper.Wrapper.ResourceWrapper.ResourceId;
+
+                if (notSetInResoureGroup && mergeWrapper.ResourceVersion > mergeWrapper.ExistingVersion)
+                {
+                    mergeWrapper.Wrapper.HasVersionToCompare = true;
+                    notSetInResoureGroup = false;
+                }
+            }


The linq version would look like this:

foreach (var wrapperWithVersionToCompare in mergeWrappersWithVersions .Where(x => x.KeepVersion && x.ExistingVersion != 0) .GroupBy(key => key.Wrapper.ResourceWrapper.ResourceId) .Select(group => group.MinBy(y => y.ResourceVersion)) .Where(x => x.ResourceVersion > x.ExistingVersion)) { wrapperWithVersionToCompare.Wrapper.HasVersionToCompare = true; }

Cool. Your version is more difficult for me to understand, so I want to stick to my current one.

Sergey, I understand you have code that functions as intended. In this case though, .net has a GroupBy function that we can leverage instead of needed to code it ourselves.

Brendan,
I agree that this code can be rewritten. I still prefer my simple loop because it seems more transparent to me.
I already ran large scale perf test on my version before submitting PR, and I would like to avoid time waste.
If you are comfortable with your logic, please make changes directly in the PR.

Remove +1 assumption for input versions in $import

3b5c022

SergeyGaluzo added Enhancement Enhancement on existing functionality. Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Schema Version backward compatible labels Jan 23, 2024

SergeyGaluzo added this to the S133 milestone Jan 23, 2024

SergeyGaluzo requested a review from a team as a code owner January 23, 2024 19:24

github-actions bot added SQL Scripts If SQL scripts are added to the PR labels Jan 23, 2024

SergeyGaluzo enabled auto-merge (squash) January 23, 2024 20:14

spelling

fc236ab

abiisnn reviewed Jan 24, 2024

View reviewed changes

order

1e06527

SergeyGaluzo disabled auto-merge January 25, 2024 01:06

SergeyGaluzo added 3 commits January 24, 2024 19:04

Abilty to add meta

8a16809

time generation

ef77c75

better time

851fc44

brendankowitz reviewed Jan 25, 2024

View reviewed changes

var name

922adf8

brendankowitz reviewed Jan 26, 2024

View reviewed changes

_ => x

ecd18ca

SergeyGaluzo enabled auto-merge (squash) January 29, 2024 20:40

brendankowitz approved these changes Jan 30, 2024

View reviewed changes

SergeyGaluzo merged commit 3046bd0 into main Jan 30, 2024
6 checks passed

SergeyGaluzo deleted the users/sergal/versionplusonefix branch January 30, 2024 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove +1 assumption for input versions in $import #3685

Remove +1 assumption for input versions in $import #3685

SergeyGaluzo commented Jan 23, 2024 •

edited

Loading

abiisnn Jan 23, 2024 •

edited by SergeyGaluzo

Loading

SergeyGaluzo Jan 24, 2024

brendankowitz Jan 25, 2024

SergeyGaluzo Jan 25, 2024

brendankowitz Jan 25, 2024 •

edited by SergeyGaluzo

Loading

SergeyGaluzo Jan 25, 2024

brendankowitz Jan 26, 2024 •

edited

Loading

SergeyGaluzo Jan 26, 2024

brendankowitz Jan 29, 2024 •

edited

Loading

SergeyGaluzo Jan 29, 2024 •

edited

Loading

brendankowitz Jan 25, 2024 •

edited by SergeyGaluzo

Loading

SergeyGaluzo Jan 25, 2024

brendankowitz Jan 25, 2024

brendankowitz Jan 26, 2024

SergeyGaluzo Jan 26, 2024 •

edited

Loading

brendankowitz Jan 30, 2024 •

edited

Loading

SergeyGaluzo Jan 30, 2024

Remove +1 assumption for input versions in $import #3685

Remove +1 assumption for input versions in $import #3685

Conversation

SergeyGaluzo commented Jan 23, 2024 • edited Loading

abiisnn Jan 23, 2024 • edited by SergeyGaluzo Loading

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

brendankowitz Jan 25, 2024 • edited by SergeyGaluzo Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankowitz Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankowitz Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

SergeyGaluzo Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

brendankowitz Jan 25, 2024 • edited by SergeyGaluzo Loading

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

SergeyGaluzo Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

brendankowitz Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyGaluzo commented Jan 23, 2024 •

edited

Loading

abiisnn Jan 23, 2024 •

edited by SergeyGaluzo

Loading

brendankowitz Jan 25, 2024 •

edited by SergeyGaluzo

Loading

brendankowitz Jan 26, 2024 •

edited

Loading

brendankowitz Jan 29, 2024 •

edited

Loading

SergeyGaluzo Jan 29, 2024 •

edited

Loading

brendankowitz Jan 25, 2024 •

edited by SergeyGaluzo

Loading

SergeyGaluzo Jan 26, 2024 •

edited

Loading

brendankowitz Jan 30, 2024 •

edited

Loading