Fix ScalarTemporalEqualityRewriter union handling and predecessor selection#5624
Open
mikaelweave wants to merge 6 commits into
Open
Fix ScalarTemporalEqualityRewriter union handling and predecessor selection#5624mikaelweave wants to merge 6 commits into
mikaelweave wants to merge 6 commits into
Conversation
When ScalarTemporalEqualityRewriter emits a bare UnionExpression as the predicate of a SearchParamTableExpression (for day-precision birthdate equality), two downstream components could not handle this shape: 1. HasUnionAllExpression / SplitExpressions checked for a union nested inside an IExpressionsContainer, so a bare union was never detected and the SQL union code path was never triggered. 2. SortRewriter had no VisitUnion override, so the base class visitor returned null for each SearchParameterExpression branch and then tried to construct new UnionExpression([null, null]), hitting the EnsureArg guard and throwing ArgumentException on every _sort=birthdate query. Fixes: - SearchParamTableExpressionExtensions.HasUnionAllExpression: return true immediately when predicate IS a UnionExpression (bare shape). - SearchParamTableExpressionExtensions.SplitExpressions: detect bare union predicate, set unionExpression = that union, allOtherRemainingExpressions = null (safe: AppendNewSetOfUnionAllTableExpressions never reads it). - SortRewriter.VisitUnion: visit all branches; return null (found signal) only if ALL branches return null; otherwise return the original reference unchanged (never reconstruct, to avoid null-in-ctor crash). Tests added: - SearchParamTableExpressionExtensionsTests: 6 tests covering HasUnionAllExpression and SplitExpressions for bare-union, nested-union, and plain-predicate shapes. - SortRewriterTests: 4 tests verifying SortWithFilter vs Sort/NotExists emission when the union covers all, none, or a subset of branches for the sort parameter; plus a baseline test for plain SearchParameterExpression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5624 +/- ##
==========================================
+ Coverage 76.91% 77.54% +0.63%
==========================================
Files 997 997
Lines 36616 36622 +6
Branches 5529 5532 +3
==========================================
+ Hits 28163 28399 +236
+ Misses 7104 6873 -231
- Partials 1349 1350 +1 🚀 New features to boost your workflow:
|
ScalarTemporalEqualityRewriter rewrites exact-day birthdate equality into a UNION ALL. When that union is followed by a Normal+Concatenation date-range pair (from DateTimeBoundedRangeRewriter/ConcatenationRewriter), SqlQueryGenerator selected the wrong predecessor CTE for the Concatenation branch: it joined its own Normal sibling instead of the shared union-aggregate CTE. Root cause: FindRestrictingPredecessorTableExpressionIndex mixed two coordinate systems - the generated-CTE counter (which a union advances by N) and indices into the unsorted _rootExpression.SearchParamTableExpressions. A union inflating the counter tripped the out-of-range guard before the Concatenation recursion could skip its sibling. Fix (Option A): record generation order explicitly in _generatedTableExpressions ((Kind, OutputCteIndex) per emitted CTE) and resolve the restricting predecessor by walking that authoritative order. A Concatenation branch resolves to its Normal sibling's predecessor, so both branches join the shared union aggregate. Documented the SortExpressionsByQueryLogic ordering invariant the walk relies on. Tests: add SqlQueryGenerator tests asserting both branches share the union-aggregate predecessor (and a non-union regression), plus a SortExpressionsByQueryLogic test asserting unions move first while the (Normal, Concatenation) sibling pair stays adjacent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mikaelweave
changed the title
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Phase 2 _generatedTableExpressions list (generation-order tracking) desynced from _tableExpressionCounter on SmartV2 include/rev-include searches, where the filtered-data CTE and SmartV2 union re-generation emit CTEs not recorded in the list. Position-based predecessor lookup then resolved to a stale/forward CTE, producing 'Invalid object name cteN'. Replace it with counter-based resolution: every kind returns _tableExpressionCounter - 1, except Concatenation which returns _tableExpressionCounter - 2 to skip its single-CTE Normal sibling so both UNION ALL branches restrict against the shared predecessor. This is byte-identical to the original generator for all non-Concatenation cases, resolving the regression, while still fixing the union+date-range pair join target. Update ADR-2606 to record the final decision. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mikaelweave
added
Bug
mikaelweave
commented
Jun 22, 2026
| } | ||
|
|
||
| private int FindRestrictingPredecessorTableExpressionIndex() | ||
| private int FindRestrictingPredecessorTableExpressionIndex(SearchParamTableExpressionKind currentKind) |
Contributor
Author
There was a problem hiding this comment.
The brittle recursive index-walker was replaced with simple, signature-driven counter arithmetic.
Concatenation was in issue in the past with Union CTEs.
mikaelweave
commented
Jun 22, 2026
| /// union, because the <see cref="UnionExpression"/> constructor rejects null children and | ||
| /// would throw if any branch visit returned null. | ||
| /// </summary> | ||
| public override Expression VisitUnion(UnionExpression expression, SqlSearchOptions context) |
Contributor
Author
There was a problem hiding this comment.
The ScalarTemporalEqualityRewriter now emits a UnionExpression, a shape SortRewriter never saw before. We need this so the base visitor can handle correctly.
…correct SQL Removes the VisitChained guard in ScalarTemporalEqualityRewriter so chained and reverse-chained exact-day birthdate equality also rewrite into the day-split UNION ALL (ADR-2605), and makes SqlQueryGenerator lower that union correctly when it is nested inside a chain. SortExpressionsByQueryLogic now only hoists ChainLevel == 0 unions (a chain-nested union stays after its chain link); AppendNewSetOfUnionAllTableExpressions propagates the parent ChainLevel and pins all branches to the shared chain-link predecessor; HandleParamTableUnion restricts ChainLevel>0 branches against that predecessor on the chain target columns. ChainLevel == 0 output is byte-identical, so SMART-scope, _type, and compartment unions are unaffected. Fixes production 500s in ICMs 21000001063947 and 815288838. Adds forward and reverse chained union SQL-generation unit tests, a deterministic ModelInfoProvider in the rewriter test harness, and an end-to-end regression (chained exact-day birthdate + base-resource sort + second chained predicate). Documents the decision in ADR-2606 (chain-aware union lowering). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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
2 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.
Summary
ScalarTemporalEqualityRewriterrewrites exact-day birthdate equality into aUNION ALL. That union shape broke two parts of the SQL pipeline; this PR fixes both, end to end, while preserving the existing date-range overlap behavior (exact-day searches still match the broader month/year-precision values — customer-visible behavior is unchanged).Phase 1 — bare union detection, splitting, and sorting
UnionExpressionpredicates inSearchParamTableExpressionExtensions(HasUnionAllExpression/SplitExpressions) so the union SQL path is correctly detected and split (scalar-temporal shape, not just the SmartV2 nested-in-MultiaryExpressionshape).SortRewriter.VisitUnionwith all-branches semantics to avoid reconstructing unions with null branches.Phase 2 — restricting-predecessor selection for union + concatenation pairs
DateTimeBoundedRangeRewriter/ConcatenationRewriter),SqlQueryGeneratorselected the wrong predecessor CTE for theConcatenationbranch — it joined its own Normal sibling instead of the shared union-aggregate CTE.FindRestrictingPredecessorTableExpressionIndexmixed two coordinate systems — the generated-CTE counter (which a union advances by N) and indices into the unsorted_rootExpression.SearchParamTableExpressions. A union inflating the counter tripped the out-of-range guard before theConcatenationrecursion could skip its sibling._generatedTableExpressions((Kind, OutputCteIndex)per emitted CTE) and resolve the restricting predecessor by walking that authoritative order. AConcatenationbranch resolves to its Normal sibling's predecessor, so both branches join the shared union aggregate.SortExpressionsByQueryLogicordering invariant (unions move first; the(Normal, Concatenation)sibling pair stays adjacent) that the predecessor walk depends on.Test Plan
SqlQueryGeneratorTestsasserting both branches share the union-aggregate predecessor (plus a non-union regression);SearchParamTableExpressionExtensionsTestscovering bare/nested-union shapes and theSortExpressionsByQueryLogicordering invariant;SortRewriterTests.Microsoft.Health.Fhir.SqlServer.UnitTestssuite green (1026/1026 on a clean run; net8.0 + net9.0).ChainingSearchTests(reverse-chain_has:ImagingStudy:patient:started+ exact-day birthdate; chained birthdate + same-target criteria;_has:Observation+ exact-day birthdate) andDateSearchTests(multi-value OR overlap,ne, accurate total, sort, leap-day — all asserting no duplicate entries and overlap semantics).