[Not Ready] SQL Birth Date Rewriter w/ overlap logic#5569
Open
mikaelweave wants to merge 60 commits into
Open
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Refs ADR docs/arch/Proposals/adr-2604-date-only-search-param-sql-optimization.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GetMapping("DATE") was coalescing with "DATETIME" and returning
typeof(FhirDateTime), masking custom search parameters whose
expressions use the .as(date) / .ofType(date) string-cast syntax.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extends ISearchParameterSupportResolver.IsSearchParameterSupported to return an IsDateOnly flag derived from FhirPath type resolution. The flag is propagated to SearchParameterInfo at startup (SearchParameterStatusManager.EnsureInitializedAsync) and at runtime custom-parameter registration (SearchParameterOperations.AddSearchParameterAsync, UpdateSearchParameterAsync). Refs ADR docs/arch/Proposals/adr-2604-date-only-search-param-sql-optimization.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collapses Core's two-column overlap predicate (StartDateTime >= startOfDay) AND (EndDateTime <= endOfDay) into a single-column equality EndDateTime = endOfDay(d) when SearchParameterInfo.IsDateOnly is true. The rewriter is gated on the metadata flag and is therefore opt-in per parameter. Composite params and range operators pass through unchanged. Refs ADR docs/arch/Proposals/adr-2604-date-only-search-param-sql-optimization.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Inserted before DateTimeEqualityRewriter so the date-only equality pattern is collapsed into a single-column equality before the generic equality rewriter would otherwise add the four-predicate overlap. After the new rewriter fires, the equality rewriter no-ops because the pattern it looks for is gone. Refs ADR docs/arch/Proposals/adr-2604-date-only-search-param-sql-optimization.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ange) Adds GivenPatientsWithDateOnlyBirthdate_WhenSearchedByEqualityAndRange_ThenCorrectPatientsAreReturned to DateSearchTests to verify that Patient?birthdate=YYYY-MM-DD (equality) and Patient?birthdate=gt YYYY-MM-DD (range) return correct results across all data stores. This serves as a regression guard for the DateOnlyEqualityRewriter introduced in ADR-2604. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the same FhirClientException/Exception catch pattern used by all other search tests in DateSearchTests.cs, so CI failures surface the server BaseAddress and Activity Id in the failure message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Promote the Date-Only Search Parameter SQL Optimization ADR from Proposals to Accepted now that implementation and testing are complete. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…riter SearchComparator.Ap emits the identical two-predicate AST shape as Eq but with constants shifted by an approximate delta. The rewriter previously matched on structure alone, so it would silently collapse an Ap query into End = approxEnd, a value that never exists in any stored row, returning zero results. Add a value-shape guard before the collapse: * startValue.TimeOfDay must be TimeSpan.Zero (Eq always starts at midnight) * endValue must equal startValue.AddDays(1).AddTicks(-1) (exactly one day) Ap constants fail the second check, so the expression passes through unchanged. Eq constants satisfy both conditions and are still collapsed. Also adds GivenApproximateDateOnlyExpression_WhenRewritten_PassesThrough unit test to assert the guard rejects the Ap-shaped expression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GetAndApplySearchParameterUpdates previously called AddNewSearchParameters without invoking the support resolver, leaving IsDateOnly=false (the default) on any custom date-only SearchParameter picked up from another instance. The optimization was therefore dead for that param until process restart. Fix: after each AddNewSearchParameters call (both the chunk-flush branch and the final-remainder branch) iterate the URLs just added, look up the SearchParameterInfo via TryGetSearchParameter, call IsSearchParameterSupported, and assign IsDateOnly from the result. Each URL is wrapped in try/catch with a logged warning so that a single resolution failure does not abort the cache refresh. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…emporal rewriter Month-precision equality must not be rewritten to a DateTimeEnd range because a stored scalar temporal value with only year precision (EndDateTime at year-end) would be excluded by a month-scoped DateTimeEnd predicate, producing false negatives. Day and year collapses are safe and are retained unchanged. IsExactMonth helper and its call site are removed. XML doc updated to explain the month pass-through rationale. A new test covers TryMatchEqualityPattern's reversed predicate order (DateTimeEnd <= end first, DateTimeStart >= start second) for the exact-day case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Limit the single-column temporal rewrite to allow-listed FHIR date parameters and keep only Patient.birthDate for now. Remove scalar temporal diagnostics and derived metadata plumbing so future date parameters can be added through the SQL rewriter allow-list. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Revert the DATE cast mapping change and remove its dedicated tests because the simplified birthdate rewrite no longer relies on derived date-only metadata. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This reverts commit df42882. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Restructure VisitSearchParameter as a top-to-bottom pipeline (allow-list check, pattern match, value extraction, precision dispatch). Replace the cascading IsExactDay/IsExactYear boolean checks with a Precision enum and ClassifyPrecision, and give the two rewrite shapes names via BuildExactDayRewrite/BuildExactYearRewrite. Public surface unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Restore ISearchParameterSupportResolver, SearchParameterOperations, and SearchParameterStatusManager to match main. These were leftover stylistic tweaks (var vs explicit tuple type, doc comment) plus an unused urlsToAdd list that was populated but never consumed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ar-temporal-birthdate-impl # Conflicts: # test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DateSearchTests.cs
Drop the branch's extra 2000-12 overlap assertion in favor of main's broader coverage. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The branch's versions still passed a 4-tuple to IsSearchParameterSupported, which no longer matches the interface (restored to a 2-tuple earlier in this branch). Aligning these mocks with main fixes the compile mismatch. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ScalarTemporalEqualityRewriter collapses day-precision birthdate equality to a single-day containment match (DateTimeEnd = endOfDay AND IsLongerThanADay = false), which deliberately excludes Patients whose stored range spans more than the searched day. Update the day-precision assertions to expect only the exact-day Patient; year and month searches still use range overlap and are unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…writer" This reverts commit f544e8b.
…nger-than-day rows
For day-precision birthdate equality, split the rewrite along the
IsLongerThanADay axis instead of replacing the predicates outright:
OR(
AND(IsLongerThanADay = false, End = endOfDay),
AND(IsLongerThanADay = true, Start >= startOfDay, End <= endOfDay))
The first branch optimizes the common day-stored case to a single
End-only equality. The second branch keeps the original two predicates
so DateTimeEqualityRewriter still expands them into the overlap form
that month/year-stored Patients rely on today, preserving current
search semantics (AB#191826).
Once the FHIR-containment fix lands, the longer=true branch becomes
dead and the rewrite can collapse back to End = endOfDay AND longer=false.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5569 +/- ##
=======================================
Coverage ? 77.32%
=======================================
Files ? 994
Lines ? 36502
Branches ? 5540
=======================================
Hits ? 28226
Misses ? 6906
Partials ? 1370 🚀 New features to boost your workflow:
|
…R for day-precision Replace the OR-based day-precision rewrite with a UnionExpression. The SQL generator already lowers UnionExpression to UNION ALL between sub-SELECTs (the same path used by CompartmentSearchRewriter and SmartCompartmentSearchRewriter), so each branch picks the appropriate filtered index instead of relying on the optimizer's OR-expansion. The two branches remain semantically identical to the previous OR formulation: the longer=false branch reduces to End = endOfDay, and the longer=true branch keeps the original Start>=X AND End<=Y pair so DateTimeEqualityRewriter still expands it into overlap form. Because the branches are mutually exclusive on IsLongerThanADay, UNION ALL (not UNION) is the correct combinator. Unit-test assertion helper updated to walk the new shape: UnionExpression(All, [SearchParameter(AND(...)), SearchParameter(AND(...))]). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add FhirSqlServerConfiguration.EnableScalarTemporalEqualityRewriter
(default false) and skip the rewriter in CreateDefaultSearchExpression
when it's off. The IConfiguration binding already routes both
appsettings ("FhirSqlServer:EnableScalarTemporalEqualityRewriter=true")
and environment variables
("FhirSqlServer__EnableScalarTemporalEqualityRewriter=true") into the
same property, so no extra wiring is needed.
Off-by-default keeps existing deployments on the original two-predicate
path; operators opt in per environment to validate the UNION ALL split
before AB#191826 lands the FHIR-containment fix.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
InProcTestFhirServer now sets FhirSqlServer:EnableScalarTemporalEqualityRewriter = true so the birthdate UNION ALL optimization is exercised end-to-end. The rewrite is behavior-preserving, so existing E2E expectations (overlap matches for partial birthdates) are unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
mikaelweave
changed the title
Provision-AcaDeploy.ps1 now sets the FhirSqlServer__EnableScalarTemporalEqualityRewriter container env var to true for SQL-backed ACA deployments. Both ci-deploy.yml and pr-pipeline.yml provision through this script, so CI and PR E2E runs exercise the birthdate UNION ALL rewrite. The rewrite is behavior-preserving, so E2E expectations are unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
3 participants
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
Describe the changes in this PR.
Related issues
Addresses [issue #].
Testing
Describe how this change was tested.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)