Implement %rowIndex environment variable#12
Merged
Merged
Conversation
Scaffolds the implement-row-index change with proposal, spec, design, and tasks for resolving the SQL on FHIR %rowIndex environment variable to a 0-based T-SQL integer across forEach, forEachOrNull, repeat, and unionAll, so the nine in-scope row_index tests can pass. Re #11
Recognise the SQL on FHIR %rowIndex environment variable in column path expressions and resolve it to the 0-based position of the current element within the active iteration. The visitor special-cases the reserved name before the user-constant lookup, returning a per-iteration index expression supplied via a new TranspilerContext.rowIndexExpr field, or 0 at the resource root. forEach and forEachOrNull set it from the OPENJSON [key] column (with a COALESCE so the null-padded forEachOrNull row reports 0). repeat sets it to a ROW_NUMBER window ordered by a zero-padded __order accumulator added to the recursive CTE, giving the depth-first pre-order position. unionAll needs no change as branches inherit or override the expression through the shared context. This makes the nine in-scope row_index.json conformance tests pass. Fixes #11
Sync the row-index-variable capability into the main specs and move the completed change to the archive.
Apply cleanup findings to the %rowIndex transpilation code: - Extract shared qualifiedKeyCols() helper, removing the duplicated partition-key column mapping in repeat.ts and cteTemplates.ts. - Collapse the orderSegment pad width into a single ORDER_SEGMENT_WIDTH constant so the zero-pad string and CAST width cannot drift. - Replace inline NVARCHAR(MAX)/NVARCHAR(4000) literals with the existing SQL_NVARCHAR_MAX/SQL_NVARCHAR_4000 constants. - Document that __order is emitted unconditionally per repeat CTE. Generated T-SQL is unchanged; all in-scope compliance tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
piotrszul
approved these changes
Jun 16, 2026
piotrszul
left a comment
piotrszul
left a comment
Collaborator
There was a problem hiding this comment.
Review: %rowIndex environment variable — Approve ✅
Reviewed the feature plus the follow-up Simplify %rowIndex CTE templates commit. Verified against a live SQL Server 2017-compatible instance.
Strengths
- Clean single-channel design:
TranspilerContext.rowIndexExprcarries the per-iteration expression; the visitor resolves%rowIndexto it (or0at the resource root), reserved before the user-constant lookup so it can't be shadowed. forEachOrNullempty-collection row correctly resolves to0viaCOALESCE.- The lexical-sort trap in
repeatis avoided via a zero-padded__orderaccumulator; pad width and CAST width now collapsed into a singleORDER_SEGMENT_WIDTHconstant so they can't drift. - Simplification (
qualifiedKeyColshelper,SQL_NVARCHAR_*constants) is byte-for-byte behavior-preserving.
Verification
- Conformance suite: 133 passed / 11 failed, all 9
row_indextests green. The 11 failures are all#experimental(fn_boundary/fn_join) and out of scope per Constitution Principle I. build,lint,format:checkall clean. Generated T-SQL is valid for SQL Server 2017+.
Minor (non-blocking)
- No single test exercises
repeatnested insideforEachwith%rowIndex— the partition-reset behavior (design Decision 3) is covered only indirectly. Optional to add.
Verdict: Spec-faithful, minimal, all in-scope tests verified green. Ready to merge.
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.
Implements the
%rowIndexenvironment variable from the SQL on FHIR v2 spec, which holds the 0-based index of the current element within the collection being iterated byforEach,forEachOrNull, orrepeat. This lets columns capture position for ordering or surrogate keys.The nine
%rowIndextests insqlonfhir/tests/row_index.json(pulled in by the submodule bump in #9) were previously failing against the MSSQL implementation and are now passing.Spec reference: https://build.fhir.org/ig/FHIR/sql-on-fhir-v2/functional-model.html#the-foreach-and-foreachornull-functions
Work was planned and tracked via an OpenSpec change, now archived.
Fixes #11