Skip scalar temporal union rewrite on chained searches#5619
Closed
mikaelweave wants to merge 10 commits into
Closed
Skip scalar temporal union rewrite on chained searches#5619mikaelweave wants to merge 10 commits into
mikaelweave wants to merge 10 commits into
Conversation
Prevent ScalarTemporalEqualityRewriter from producing day-split UNION expressions while visiting chained search predicates, while preserving the direct birthdate rewrite path. Add SQL Server chained birthdate E2E coverage and a unit guard for chained visitor context. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
## What\n- Skip ScalarTemporalEqualityRewriter day-split union rewrite when the root expression contains any chain expression (forward or reverse).\n- Add regression unit tests covering both reverse-chain and forward-chain root query shapes to ensure birthdate equality stays pass-through.\n- Add an E2E forward-chain ImagingStudy started + patient birthdate + tag POST search test that mirrors the reverse-chain scenario.\n\n## Why\nUnion-expanded CTEs can shift predecessor selection in SqlQueryGenerator for complex chain queries. Skipping the scalar union rewrite for chain-containing roots preserves expected CTE ordering and prevents joins from binding to the wrong CTE branch.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Improved comments in SqlQueryGenerator and ScalarTemporalEqualityRewriter to clarify UNION ALL expansion, SMART scope unions, and DOB day-split handling, documenting current limitations and guardrails. - Refactored chaining search E2E test to use unique tenant tags and dynamic patient creation; removed redundant forward chain test. - Cleaned up Fixture class by removing static tenant tag and patient fields now handled within tests.
Add SQL-only E2E tests for partial birthdate OR/ne/total/sort and leap-day cases in DateSearchTests, plus chained birthdate month/day scenarios in ChainingSearchTests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mikaelweave
force-pushed
the
personal/mikaelw/fix-dob-rewriter-has
branch
from
41d5968 to
6d9df0e
Compare
…o local variable' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…o use Where' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
Author
|
@copilot resolve the merge conflicts in this pull request |
Contributor
Conflicts are resolved after merging |
mikaelweave
added
Bug
| } | ||
|
|
||
| _disposed = true; | ||
| await _searchParameterOperations.DeleteSearchParameterAsync(resource.RawResource, cancellationToken, true); |
Comment on lines
+108
to
+113
| ResourceWrapperFactory wrapperFactory = Mock.TypeWithArguments<ResourceWrapperFactory>( | ||
| new RawResourceFactory(new FhirJsonSerializer()), | ||
| new FhirRequestContextAccessor(), | ||
| _searchIndexer, | ||
| _searchParameterDefinitionManager, | ||
| Deserializers.ResourceDeserializer); |
| WHERE Uri = @uri", | ||
| connection); | ||
| await using SqlConnection connection = await _sqlConnectionBuilder.GetSqlConnectionAsync(cancellationToken: cancellationToken); | ||
| var command = new SqlCommand("DELETE FROM dbo.SearchParam WHERE Uri = @uri", connection); |
Comment on lines
+62
to
+131
| { | ||
| // Arrange | ||
| const int concurrentOperations = 5; | ||
| var executionOrder = new List<int>(); | ||
| var lockObject = new object(); | ||
| var entryBarriers = new List<TaskCompletionSource<bool>>(); | ||
| var continueSignals = new List<TaskCompletionSource<bool>>(); | ||
|
|
||
| // Create synchronization primitives for each operation | ||
| for (int i = 0; i < concurrentOperations; i++) | ||
| { | ||
| entryBarriers.Add(new TaskCompletionSource<bool>()); | ||
| continueSignals.Add(new TaskCompletionSource<bool>()); | ||
| } | ||
|
|
||
| // Act | ||
| var tasks = new List<Task<int>>(); | ||
| for (int i = 0; i < concurrentOperations; i++) | ||
| { | ||
| var operationId = i; | ||
| var entryBarrier = entryBarriers[i]; | ||
| var continueSignal = continueSignals[i]; | ||
|
|
||
| tasks.Add(SearchParameterConcurrencyManager.ExecuteWithLockAsync(TestUri1, async () => | ||
| { | ||
| lock (lockObject) | ||
| { | ||
| executionOrder.Add(operationId); | ||
| } | ||
|
|
||
| // Signal that this operation has entered the critical section | ||
| entryBarrier.SetResult(true); | ||
|
|
||
| // Wait for permission to continue (controlled by test) | ||
| await continueSignal.Task; | ||
|
|
||
| return operationId; | ||
| })); | ||
| } | ||
|
|
||
| // Verify operations execute sequentially by controlling their execution | ||
| for (int i = 0; i < concurrentOperations; i++) | ||
| { | ||
| // Wait for the next operation to enter the critical section | ||
| await entryBarriers[i].Task; | ||
|
|
||
| // Verify only this operation has executed so far | ||
| lock (lockObject) | ||
| { | ||
| Assert.Equal(i + 1, executionOrder.Count); | ||
| Assert.Equal(i, executionOrder[i]); | ||
| } | ||
|
|
||
| // Allow this operation to complete | ||
| continueSignals[i].SetResult(true); | ||
| } | ||
|
|
||
| var results = await Task.WhenAll(tasks); | ||
|
|
||
| // Assert | ||
| Assert.Equal(concurrentOperations, results.Length); | ||
| Assert.Equal(concurrentOperations, executionOrder.Count); | ||
|
|
||
| // Verify all operations completed in the correct order | ||
| for (int i = 0; i < concurrentOperations; i++) | ||
| { | ||
| Assert.Equal(i, executionOrder[i]); | ||
| Assert.Equal(i, results[i]); | ||
| } | ||
| } |
| { | ||
| private readonly ISearchParameterValidator _searchParameterValidator; | ||
| private readonly RequestContextAccessor<IFhirRequestContext> _fhirRequestContextAccessor; | ||
| private ISearchParameterValidator _searchParameterValidator; |
Comment on lines
+1152
to
+1160
| foreach (Type type in types) | ||
| { | ||
| // Filter out the extension converter because it will be added to the converter dictionary in the converter manager's constructor | ||
| if (type.Name != nameof(FhirTypedElementToSearchValueConverterManager.ExtensionConverter)) | ||
| { | ||
| var x = (ITypedElementToSearchValueConverter)Mock.TypeWithArguments(type, referenceSearchValueParser, codeSystemResolver); | ||
| fhirElementToSearchValueConverters.Add(x); | ||
| } | ||
| } |
mikaelweave
force-pushed
the
personal/mikaelw/fix-dob-rewriter-has
branch
from
c5c6827 to
7ac0d3e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a skip for the DOB rewriter for more chained / reverse chained queries.
Adds more tests for this rewriter.
Testing
local testing
e2e
unit
AB#195112
FHIR Team Checklist
Semver Change (docs)
Patch