Skip to content

Add Optimized Single Column Date Rewriter #5562

Open
mikaelweave wants to merge 54 commits into
mainfrom
personal/mikaelw/scalar-temporal-birthdate-impl
Open

Add Optimized Single Column Date Rewriter #5562
mikaelweave wants to merge 54 commits into
mainfrom
personal/mikaelw/scalar-temporal-birthdate-impl

Conversation

@mikaelweave
Copy link
Copy Markdown
Contributor

@mikaelweave mikaelweave commented May 12, 2026

Description

  • Adds an allow-listed SQL scalar-temporal equality rewriter for Patient.birthdate.
  • Rewrites exact-day and exact-year equality searches to use DateTimeEnd predicates while preserving existing handling for month precision, ranges, composites, and non-allow-listed date parameters.
  • Adds focused unit coverage for the rewrite behavior and includes the current partial-birthdate E2E baseline from the test PR.

Related issues

AB#190492

Testing

Describe how this change was tested.

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 65 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
  • Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
  • Tag the PR with Schema Version backward compatible or Schema Version backward incompatible or Schema Version unchanged if this adds or updates Sql script which is/is not backward compatible with the code.
  • When changing or adding behavior, if your code modifies the system design or changes design assumptions, please create and include an ADR.
  • CI is green before merge Build Status
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

mikaelweave and others added 30 commits April 24, 2026 10:40
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>
mikaelweave and others added 12 commits May 7, 2026 14:00
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>
@mikaelweave mikaelweave requested a review from a team as a code owner May 12, 2026 05:13
@mikaelweave mikaelweave added this to the FY26\Q4\2Wk\2Wk23 milestone May 12, 2026
@mikaelweave mikaelweave added Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Enhancement Enhancement on existing functionality. Area-SQL Area related to the SQL Server data provider ADR-Included ADR Included in the PR Schema Version backward compatible No-PaaS-breaking-change labels May 12, 2026
mikaelweave and others added 6 commits May 11, 2026 22:21
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>
Comment on lines +45 to +47
(MultiaryExpression)Expression.And(
Expression.GreaterThanOrEqual(FieldName.DateTimeStart, null, start),
Expression.LessThanOrEqual(FieldName.DateTimeEnd, null, end));
Comment on lines +144 to +146
var reversedPattern = (MultiaryExpression)Expression.And(
Expression.LessThanOrEqual(FieldName.DateTimeEnd, null, EndOfDay),
Expression.GreaterThanOrEqual(FieldName.DateTimeStart, null, StartOfDay));
Comment on lines +184 to +186
(MultiaryExpression)Expression.And(
new BinaryExpression(BinaryOperator.Equal, FieldName.DateTimeEnd, endPredicate.ComponentIndex, endPredicate.Value),
Expression.Equals(SqlFieldName.DateTimeIsLongerThanADay, endPredicate.ComponentIndex, false));
Comment on lines +189 to +191
(MultiaryExpression)Expression.And(
Expression.GreaterThanOrEqual(FieldName.DateTimeEnd, endPredicate.ComponentIndex, yearStart),
Expression.LessThanOrEqual(FieldName.DateTimeEnd, endPredicate.ComponentIndex, yearEnd));
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>
@brendankowitz
Copy link
Copy Markdown
Member

🤖 Automated Pipeline Failure Analysis — Build #49384

❌ Failed Jobs

Job Result
Run R4 Cosmos Tests cosmosE2eTests ❌ Failed
Run Stu3 Cosmos Tests cosmosE2eTests ❌ Failed
All SQL E2E tests (R4, R5, Stu3) ✅ Passed
All unit tests & integration tests ✅ Passed

Root Cause

The ScalarTemporalEqualityRewriter was registered in the SQL Server search pipeline (SqlServerSearchService.cs, line ~2062), but the Cosmos DB search pipeline (FhirCosmosSearchService.cs, lines 78-84) still uses only:

compartmentSearchRewriter,
smartCompartmentSearchRewriter,
DateTimeEqualityRewriter.Instance,

The shared E2E test GivenPatientsWithPartialBirthdates_WhenSearchedByEquality_ThenCurrentOverlapBehaviorIsPreserved in DateSearchTests.cs was updated to expect the tighter day-precision results (e.g. dayBundle → only patient2000March03), which is correct for SQL where the rewriter is active. However, Cosmos DB still returns the old overlap-based results (patient2000, patient2000March, patient2000March03), causing both Cosmos E2E test jobs to fail.

Suggested Fix

Since the test class is decorated with [HttpIntegrationFixtureArgumentSets(DataStore.All, Format.Json)] and extends SearchTestsBase<DateSearchTestFixture>, the Fixture.DataStore property is available to branch assertions by backend. For example:

Bundle dayBundle = await Client.SearchAsync(ResourceType.Patient, $"birthdate=2000-03-03&_tag={tag}");
if (Fixture.DataStore == DataStore.SqlServer)
{
    // SQL path: ScalarTemporalEqualityRewriter narrows to exact-day containment
    ValidateBundle(dayBundle, patient2000March03);
}
else
{
    // Cosmos path: still uses range overlap (tracked by AB#191826)
    ValidateBundle(dayBundle, patient2000, patient2000March, patient2000March03);
}

Apply the same pattern for the lastDayBundle assertion.

Alternatively, the rewriter could be added to the Cosmos DB pipeline too, but ScalarTemporalEqualityRewriter uses SQL-specific constructs (SqlFieldName.DateTimeIsLongerThanADay), so that would require additional Cosmos DB query model work.


Analysis by Azure SRE Agent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ADR-Included ADR Included in the PR Area-SQL Area related to the SQL Server data provider Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Enhancement Enhancement on existing functionality. No-PaaS-breaking-change Schema Version backward compatible

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants