Skip to content

Collapse the four data operations into $sql-run and $sql-export - #392

Merged
niquola merged 34 commits into
mainfrom
align-operation-parameters
Aug 6, 2026
Merged

Collapse the four data operations into $sql-run and $sql-export#392
niquola merged 34 commits into
mainfrom
align-operation-parameters

Conversation

@johngrimes

@johngrimes johngrimes commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

This PR started by bringing the four data operations into agreement with one another. Issue #394 argues that the alignment work exposed the split as unnecessary: once aligned, the two export operations differ only in the name of the container parameter, the name of the dependency parameter and where parameter binding lives, and the same is true of the two run operations. The later commits act on that. Four operations become two, invoked at the system level, each acting on a subject that may be a ViewDefinition, a SQLQuery Library or a SQLView Library.

Closes #383, closes #384, closes #389, closes #391, closes #394.

The collapse is not only a reduction in surface. $sql-export takes a repeating complex subject parameter, so a mixed set of views and queries is exported as one job, under one set of filters, against one snapshot, with one manifest carrying one output entry per subject. That is the one thing a client can do here that it could not do before: two separate export jobs see the data at two different points in time, so a view output and a query output could not safely be joined. $sql-run keeps its subject as flat primitive parameters so GET survives.

Breaking change

This is a breaking change for anyone building against the CI build. None of the four operations appears in a published version of the specification (see https://sql-on-fhir.org/ig/history.html), so no conforming implementation of a release is affected and Constitution IV owes no deprecation period. But @oloveluck reported passing tests against the contract in the earlier commits of this PR, and that contract no longer exists: the operation codes, the subject parameter names, the dependency parameter name and the invocation levels have all changed. @niquola's approval predates the collapse and should be treated as stale; re-review is requested.

There is no alias, no retired stub and no redirect page for any removed operation. Their content remains recoverable from git history.

Parameter mapping

Was Now
$viewdefinition-run, $sqlquery-run at system, type and instance level $sql-run at system level only
$viewdefinition-export, $sqlquery-export at system, type and instance level $sql-export at system level only, POST only
viewCanonical / viewReference / viewResource subjectCanonical / subjectReference / subjectResource
queryCanonical / queryReference / queryResource the same three parameters
Repeating view with name / viewCanonical / viewReference / viewResource Repeating subject with name / subjectCanonical / subjectReference / subjectResource / parameters
Repeating query with name / queryCanonical / queryReference / queryResource / parameters The same repeating subject
Subject named by the request path at instance level Always named by a subject* parameter
tableSource (per invocation, SQLQuery operations only) context (job-wide, both operations)
Top-level parameters on $sqlquery-export, instance level only Removed; binding is always per subject
resource on $viewdefinition-run resource on $sql-run, permitted only where the subject is a ViewDefinition
_limit on the two run operations _limit on $sql-run
clientTrackingId on the two export operations clientTrackingId on $sql-export
patient, group, _since, source, _format, header on all four Unchanged in meaning on both
Per-parameter scope lists Removed; there is one invocation level
Two jobs to export views and queries together, with no consistency between them One job, one snapshot
A shared dependency supplied once per job, twice in all Supplied once

GET support, the format enumeration, filtering, _limit, source, resource and Bundle unwrapping are unchanged in behaviour.

Three decisions a reviewer should weigh

These are normative choices rather than corrections, so they are called out here rather than left to be discovered in the diff.

1. Both operations are system level only. system is true, type and instance are false, and there is no OperationDefinition.resource element, since that element lists the types an operation can be executed on and is meaningful only where type or instance invocation is offered. Issue #394 proposes keeping all three levels with a repeating resource; that keeps every invocation available today but retains all of the instance-level machinery - the subject named by the request path, the per-parameter scope lists, the rule that no subject parameter may be supplied at instance level, and the instance-only top-level parameters this PR had to introduce - and adds a new oddity, in that ViewDefinition/$sql-export naming a SQLQuery subject would be well formed but nonsensical. Two consequences follow. The CapabilityStatement guidance now declares both operations in CapabilityStatement.rest.operation rather than under a resource type; the ViewDefinition and Library rest.resource entries survive for their CRUD and search interactions, carrying no operation entries. And the note saying the run operation is system level only in FHIR R4 compatibility mode and type and instance level in R6 mode is removed, since system-level-only invocation makes the distinction moot.

2. The dependency parameter is named context. It carries artefacts the server cannot itself resolve, matched to the subjects' dependencies by canonical URL, and it accepts inline resources only. The name collides with established FHIR usage: CQL's $evaluate and CDS Hooks both use context for the subject of evaluation rather than for supporting artefacts, so a FHIR practitioner may read it as "the patient this runs for". That is accepted as a considered trade-off, and the parameter documentation states plainly what it carries. What the name buys is that it carries no hint of artefact type, so admitting further kinds later - terminology artefacts among them - needs only a widening of the accepted targetProfile list, with no rename and no second parameter. dependency was the alternative most seriously considered, and is equally type-neutral. tableSource was rejected because a terminology artefact supplied under it would read as nonsense, which also answers the review comment on this PR that something felt wrong about that name.

3. The single-snapshot requirement is a SHALL. A server SHALL compute every subject in one $sql-export job against a single consistent view of the data. This is the requirement that makes one job better than two, and its absence is one of the four problems #394 lists with the status quo; a weaker verb would leave clients unable to rely on the property the operation exists to provide. It is testable by observation: mutate the dataset while a job runs and join two of its outputs. A SHOULD with server documentation would be kinder to stores without snapshot isolation, but hollows out the justification for the change. Note what is not constrained: a canonical URL reached from two subjects is resolved once, so both subjects see the same artefact, but whether that artefact is then materialised once or several times remains implementation-defined, consistent with the specification's existing position on materialisation strategy.

Ballot timing

This lands in the current ballot cycle. The four operations are unpublished, so replacing them breaks no conforming implementation, and the window for doing so without a deprecation period closes with this ballot; shipping a ballot with four operations that everyone agrees should be two would guarantee the deprecation period later. Issue #394 frames the collapse as post-ballot STU work, and that framing is being set aside deliberately rather than overlooked. Reviewers who disagree should say so - the collapse commits are separable from the alignment commits that precede them.

The alignment work this PR also carries

The earlier commits remain, and their substance survives the collapse:

  • An unresolvable patient or group returns 400 Bad Request, not 404 Not Found. The shared page states the principle: an artefact the operation is about, or requires in order to run, yields 404 when unresolvable, so an unresolvable subject or dependency stays at 404; a value that merely scopes the data yields 400. issue.code remains not-found and expression names the parameter.
  • GET is documented for the primitive parameter subset, which now covers a query subject as well as a view subject.
  • operations-materialize.md is removed. It specified a $materialize operation, with a declared canonical URL, for which no OperationDefinition exists anywhere in input/fsh/; it is absent from the menu, nothing links to it, and its content contradicts the shared behaviour page. That removal is its own commit and can be dropped without disturbing anything else.
  • operations-common.md opens every section with an explicit statement of which operations it governs, so a missing line is visibly a defect. The vocabulary is now two-way: both operations, the run operation only, or the export operation only. The table of parameters not offered on every operation is down from five rows to four - _limit, resource, clientTrackingId and the subject shape - because the two rows that existed only to explain differences between two operations of the same kind no longer have anything to explain.
  • The glossary gains snapshot, manifest and subject. "Snapshot" is a database term newly load-bearing in normative text; the other two were already relied upon without a definition.

Build and testing

CI will fail on this branch, because it already fails on main. The comparison that matters is against this branch immediately before the collapse commits, built locally with the same IG Publisher jar (v2.2.10) on the same machine roughly three hours apart, so it is a controlled before-and-after:

Before After
Errors 37 37
Unsuppressed warnings 18 18
Hints 57 53
Broken links 0 0

input/ignoreWarnings.txt is unchanged: no suppression was added. Of the 37 errors, 36 are ViewDefinition example-validation failures (#377) and one is the missing Jira specification artefact (#378). No error or warning concerns an OperationDefinition; the only qa.html messages that mention one are information-severity, and are the same deprecated-jurisdiction-value-set hints the four previous OperationDefinitions produced.

Verification beyond the build had three layers. Assertions covering the mechanically checkable outcomes - zero occurrences of each removed operation code and canonical URL under input/ and in sushi-config.yaml, exactly two generated OperationDefinitions, exactly four operation pages, system/type/instance and the absence of resource on each definition, the parameter name set of each matching its written contract, and no parameter carrying a scope - were written and confirmed failing before any source was edited, and all 34 now pass. SUSHI compiles with zero errors. Each end-to-end scenario was then walked against the built pages to confirm the request shape it uses is documented and accepted by the generated OperationDefinitions.

The shared suite in sql-on-fhir.js evaluates ViewDefinitions against JSON fixtures and has no notion of an HTTP request, a status code, an operation invocation or an asynchronous job, so the subject-naming rules, the error contract and the single-snapshot requirement have nothing to be expressed against. Constitution principle III is therefore unsatisfied for this layer, and is recorded as a known gap rather than glossed over. Building that harness spans a second repository and is a project in its own right.

Reading the commits

Read them in order. The alignment commits come first and stand on their own; the collapse commits follow, and are additive before they are subtractive - the two new OperationDefinitions land first, then each page pair, then the shared page, then the referencing pages and the menu, and only then the removal of the four ViewDefinition pages. Every commit builds, so bisecting the branch stays useful, and the removal is isolated in its own commit so a reviewer who disputes it can drop that one commit without disturbing anything else. The first commit of all converts the OperationDefinitions to FSH soft indexing and is provably a no-op; without it, inserting one parameter renumbered eighteen others and buried every later change.

The four OperationDefinition instances used explicit positional indices up
to parameter[19]. Inserting one input parameter renumbered every later
one, so each substantive change would carry a long mechanical tail that
buries the actual edit from reviewers. Soft indexing removes that.

Purely mechanical: no parameter is added, removed or altered. Verified by
regenerating fsh-generated/resources and confirming the output is
byte-identical to the output before the change.

Also replaces emdashes with spaced hyphens in the section comments, per
the project writing style. Comments are not emitted, so this does not
affect the generated output either.
Adds a canonical-typed parameter to each of the four data operations, so
that an artefact can be named by its canonical URL conformantly:
viewCanonical on the two view operations, queryCanonical on the two query
operations, as a part of the repeating view/query parameter on the export
operations.

The Reference parameters previously documented "a canonical URL" as one of
their accepted values. Reference.reference is not a slot base FHIR permits
a canonical URL to occupy, so the documented usage produced a
non-conformant request; and "a canonical URL" and "an absolute URL" were
listed as distinct forms of the same string with no way to tell them
apart. Each Reference parameter is now narrowed to a literal location, and
the type carries the meaning instead of the shape of the string.

Deletes the three-step recommended resolution algorithm from both view
operation pages. Its step 2 was precisely the conflation the new parameter
replaces, so under a narrowed Reference the page contradicted itself.
Resolution is now an implementation matter a server declares rather than
one this guide prescribes.

Adds "Declaring partial operation support" to the CapabilityStatement
page, citing the base FHIR mechanism for
CapabilityStatement.rest.resource.operation.definition, and cites it from
all four operation pages. The guide already required servers to document
supported reference formats without saying how.

Closes #389
Replaces the complex view parameter with a plain repeating viewResource
parameter accepting an inline ViewDefinition or SQLView.

The view parameter's viewReference part duplicated what relatedArtifact
already carries: a SQLQuery identifies its table sources by canonical URL
in relatedArtifact.resource, with the SQL table name in label, so a
client-supplied reference to a server-resolvable artefact added nothing but
a conflict to adjudicate. Its name part was unused, because supplied views
produce no output entries and so have nothing for a friendly name to name.
With both parts gone, a complex parameter wrapping one part is structure
without content.

The parameter now also accepts a SQLView, not just a ViewDefinition, since
a SQLQuery's relatedArtifact entries may name either, and it applies at the
instance level as well: a stored query can depend on a view the server
cannot resolve.

Adds "ViewDefinition table sources" to operations-common.md, stating the
matching algorithm, the precedence of supplied resources over
server-resolvable ones, the error cases, and what stays
implementation-defined. Written once there and cited, rather than
duplicated on both query operation pages.

Closes #384
Adds the repeating viewResource parameter to $sqlquery-run with the same
type, cardinality, scopes and targetProfile values it carries on
$sqlquery-export, so the two operations agree on how a client supplies a
table source the server cannot resolve.

The fully ad-hoc workflow - inline query, inline dependency views, nothing
stored on the server - was previously impossible on the synchronous
operation, which is the one it most naturally belongs to. The export
operation offered a slot for it and the run operation offered none.

Adds worked examples for the ad-hoc case, for a supplied SQLView whose own
dependency is also supplied, for the rejection of an unmatched entry, and
for supplying a table source at instance level. Cites the shared matching
rules rather than restating them, and corrects step 1 of the execution flow
to account for supplied resources taking precedence.

Closes #383
Raises patient from 0..1 to 0..* on $viewdefinition-run, and adds patient
(0..*), group (0..*) and _since (0..1) to $sqlquery-run, so all four data
operations carry the same filtering triple with the same meaning. Moving
between synchronous and asynchronous execution is now purely a change of
operation.

$sqlquery-run previously accepted none of the three while $sqlquery-export
accepted all three. The patient cap of 0..1 on $viewdefinition-run bounded
nothing in practice, because group was already unbounded there, so it only
made the obvious way of naming two patients unavailable and pushed clients
into creating a Group.

States on both query operations that the filter applies to the resources
feeding the dependency views, before the SQL executes, so the SQL sees
tables already narrowed to the requested scope.

Closes #391
Moves the patient, group and _since clarifications to a Filtering section
on operations-common.md with stable anchors, and replaces the copies on the
four operation pages with citations of it.

The clarifications were already near-identical on three pages, and
requiring them to be identical on four would have institutionalised the
duplication that let them drift in the first place. operations-common.md
already carries output formats, return representation, content negotiation,
streaming and asynchronous delivery on the same basis.

Also states there that _limit is not one of the filtering parameters: it
caps the rows returned to the client rather than constraining the data a
view sees. Two page summaries had grouped it with the filters.
Pre-existing defects in the rendered parameter tables, independent of the
four issues this branch closes but in the same tables.

The $viewdefinition-run parameter tables omitted the system level from
every scope column, although the operation declares system = true and its
own introduction states that R4 compatibility mode supports the system
level only. Eight rows are corrected.

The viewReference row on that page claimed a scope of "type, instance"
while the footnote immediately below it said the parameter is not allowed
at instance level. The two now agree; that row was rewritten when the
canonical parameters were added, so this commit only completes the
remaining tables.

The $viewdefinition-export page declared instance = true but never stated
what an instance-level invocation does, unlike $sqlquery-export, which
does. It now states that the ViewDefinition identified by the request path
is the export subject and that the view parameter does not apply there.
Pre-existing defect, discovered during the consistency pass.

Both export operations declare instance = true, yet every input parameter
was scoped to system and type only, so by the definitions an instance-level
export accepted no input parameters at all: not _format, not patient, not
even clientTrackingId. The $sqlquery-export prose said the opposite,
promising that the bound Library may be "optionally combined with the export
control, filtering, and data source parameters listed below".

Corrects the definitions rather than the prose, because that is the side
that leaves a useful invocation level: an export that cannot specify its
output format is not worth invoking, and the run operations already show the
intended shape, scoping everything except the subject to all three levels.

The subject parameters, view and query with their parts, stay at system and
type, because at instance level the subject is the request path.
Completes the documentation pass over pages that reference the reshaped
parameters.

operations.md described the $sqlquery-run query source as "supplied inline,
by reference, or inferred from the URL path", which is now incomplete, and
listed $viewdefinition-run as invocable at the type and instance levels
only, omitting the system level that the operation declares. Both are
corrected.

The SQLQuery profile page describes the dependency graph but not how it is
resolved at invocation time, so it now points at the shared table sources
section.

Also moves the instance-level discussion on $sqlquery-export below the
parts table it was interrupting, and replaces the remaining emdashes in
the touched pages with spaced hyphens per the project writing style.
FR-027 requires the parameter asymmetries that survive this alignment to be
stated with their reason rather than left as an apparent oversight. The
_limit case was explained; the resource case was not stated anywhere.

Adds "Parameters that do not apply to every operation" to
operations-common.md, covering both: _limit caps rows in an operation
response, so an export that delivers files has nothing to cap; resource
carries inline FHIR resources to transform, and accepting it on the SQLQuery
operations would need its own semantics for how supplied resources reach
each dependency view, so it is deliberately deferred.

Cites that section from $sqlquery-run, where a reader who has just met
viewResource is most likely to wonder why there is no resource parameter,
and distinguishes the two: viewResource supplies the views a query reads
from, not the resources those views project.

Also extends the $viewdefinition-export HTTP status table with the
subject-form 400 and 404 clauses, matching the treatment $sqlquery-export
already had.
Three gaps against the common error contract in the specification's
contracts/operation-parameters.md.

$sqlquery-export said only that the query parameter "does not apply" at
instance level and that supplying it would be "out of scope", never that the
request is rejected. Its sibling $viewdefinition-export states the rejection,
so the two implied different behaviour for the same condition. The 400 row
and the instance-level prose now name it.

$viewdefinition-run contradicted the other three operations on an unknown
patient or group: its worked example returned 400 Bad Request while the
contract, and the wording this branch added to the other three pages, make it
404 Not Found. The example and the 404 row are corrected, with a pointer to
the shared filtering section.

The system-level example on $sqlquery-run passed parameters as an undeclared
"parameter" input with name and value parts. The declared mechanism is the
"parameters" parameter carrying a nested Parameters resource, which every
other example on the page already used.
The informative OperationDefinition in the partial-support section omitted
name, which is 1..1, so an implementer copying it verbatim would produce an
invalid resource.

Both export operations carry a 404 "Patient or Group Not Found" example but
omitted the condition from their 404 status rows, leaving an asymmetry with
the run pages. All four rows now name it.
@johngrimes
johngrimes requested review from niquola and smunini July 28, 2026 10:16
On $sqlquery-run and $sqlquery-export the parameter carrying inline
dependency table sources was named viewResource, which is also the name
of the parameter carrying the subject on $viewdefinition-run. The two
differ in cardinality, in invocation scope and in meaning, so a client
generated from the definitions saw one name for two roles.

Renames it to tableSource, matching the "ViewDefinition table sources"
section title, and records why no tableSourceCanonical or
tableSourceReference sibling exists: dependencies are matched by
canonical URL, so a parameter naming one that way could only hand the
server a URL it has already failed to resolve.

The parameter is introduced by this branch and appears in no published
version, so the rename breaks nothing.
The shared filtering section said only that a server SHOULD return
details in an error response, while all four operation tables said 404
and one page said neither. No page stated which code applied, and 404
contradicts base FHIR's own use of it: HTTP ties 404 to the request
target, which at system and type level is the operation endpoint rather
than the patient.

States the rule once in the common page, along with the principle that
separates the two codes: an artefact the operation is about, or requires
in order to run, yields 404 when unresolvable; a value that merely scopes
the data yields 400. Subject and dependency failures therefore stay at
404. The four status-code tables and the worked examples now agree, and
each rejection names the offending parameter in expression.

This is a normative change and is called out as contestable in the pull
request body.
The page mixed scoped and unscoped sections: Streaming scoped itself
explicitly, Return Representation scoped itself in its opening sentence,
Content Negotiation not at all, and Output Formats claimed its rules
applied to every operation when one of them was wrong for exports. That
inconsistency is how the defect entered.

Gives every section an explicit "Applies to" line and normalises the
ad-hoc statements to the same form, so a missing line is now visibly a
defect. Splits the _format default rule, since Accept describes the
response the client is about to receive and only the run operations
return the result there; on an export, omitting _format yields ndjson
irrespective of Accept. Scopes Content Negotiation to the run operations
for the same reason, and adds the matching Accept-precedence line to the
SQLQuery export page.

Completes the asymmetry table with clientTrackingId, tableSource and
parameters, so it now lists every input parameter absent from at least
one operation or offered in a different shape. Gives _limit one home in a
new run-only section that both run pages reference rather than restate,
and qualifies the claim that a $sqlquery-run body is accepted by
$sqlquery-export by naming the three parameters that do not carry across.
parameters existed only as a part of query, and query does not apply at
instance level, so exporting a parameterised stored query asynchronously
was impossible even though the specification told the reader that moving
between synchronous and asynchronous execution is purely a change of
operation.

Adds a top-level parameters (Parameters, 0..1) scoped to the instance
level alone. Restricting it there avoids every ambiguity a system or
type level form would raise, since at those levels several queries may be
present, each declaring its own parameters. Supplying it at system or
type level is a 400.

The page now describes the slot rather than stating that none exists, and
carries a worked example.
The page said the operation is invoked with POST, and the intro's
endpoint table showed POST at every level. $sqlquery-run is safe, and
queryCanonical plus _format plus the filters are all primitive, which is
exactly the case base FHIR permits over GET. Only the prose forbade it,
so this removes an asymmetry with $viewdefinition-run rather than adding
capability.

Adds an HTTP methods section matching the one on the ViewDefinition run
page, listing the parameters available over GET and stating that
parameters, tableSource and queryResource require POST. Adds two worked
GET examples, with the version-pinning | percent-encoded as %7C, and
aligns both run intros so they describe methods in identical terms.
view.viewCanonical was declared in the OperationDefinition but
demonstrated nowhere, making it the only one of the four new canonical
parameters without a worked example. Adds a kick-off example naming a
view by canonical URL with a pinned version, and notes what omitting the
suffix means.

Rewrites the export intro, the only one of the four not updated when the
others were aligned, to the same three-column endpoint table, naming the
subject-source parameters at each level. Marks the storage products it
mentioned as informative rather than leaving them in prose that reads as
normative.
A reader comparing a prose table against the rendered OperationDefinition
found disagreements. Individually small, together they are what makes the
tables untrustworthy.

FSH: both export operations' _format output parameter bound a value set
containing fhir, which an export cannot emit, and now binds the
export-only enumeration. view carries the same invocation-level
explanation query already had. ViewDefinitionExport declares
versionAlgorithmString as the other three do. _since and source, which
read four and three different ways, now read identically on all four.

Prose: the parts table on the ViewDefinition export page lists only the
parts of view, matching its SQLQuery counterpart. Both export pages point
at the guide's partial-support mechanism instead of restating a weaker
version of it. The inline SQLQuery example carries its SQL in the
sql-text extension rather than in Attachment.title. Every prose table
declaring a type of ViewDefinition now footnotes an explanation of the
CanonicalResource declaration and the role of targetProfile, added to the
common page. That page also uses one spelling convention throughout, the
US one already used by the guide's prose and page titles.

Index page: drops the claim that the run operations support "table
formats", which the enumeration does not define, and describes both
export operations' subject-naming in the same terms as the two run
summaries.
The four notes pages did not agree on where their heading hierarchy
started: the SQLQuery run page began at h3 while its three siblings began
at h4. Several sections were also introduced at their parent's level
rather than beneath it, so a section that reads as a wrapper rendered as a
sibling of the content it introduces, and one section was empty.

Demotes every heading on the SQLQuery run page by one level, so all four
now start at the same place. Kramdown derives slugs from heading text
rather than level and explicit anchors are unaffected, so no inbound link
breaks; this was confirmed by a link check rather than assumed.

Removes the empty Request Format section, and places Input Parameters,
Successful Requests and Common Error Scenarios above the sections they
introduce on every page that had them flat.

This structure was already inconsistent before this branch; it is
corrected here because the branch's whole subject is uniformity across
these four pages.
The page specified a $materialize operation, complete with a declared
canonical URL, that the guide does not define:

- No OperationDefinition for it exists anywhere in input/fsh/.
- It is absent from the menu in sushi-config.yaml.
- Nothing links to it, so it is unreachable by navigation.
- Its content now contradicts the shared behaviour page on completion
  signalling (200 OK on the status poll rather than 303 plus a result
  URL), on filtering, which it duplicates locally, and on instance-level
  handling, where it gives three different rules including an internal
  contradiction between "SHALL be ignored if provided" and "not allowed
  at instance-level".

An unreachable page describing an undefined operation misleads anyone who
reaches it. Removing it is proposed here rather than done silently: the
reasons are in this message and in the pull request body, and the content
remains recoverable from git history.
Three residual inconsistencies the review found in work this branch
already touches:

- operations-common.md kept "artefact" alongside the US forms it was
  normalised to. The rest of the guide already uses "artifact", so the
  five operation pages now match it and FR-036 holds without introducing
  a cross-page difference.
- The four _since prose table cells carried two wordings, even though the
  OperationDefinition documentation had been made identical. They now
  restate the definition rather than paraphrasing it two ways.
- GET Method Limitations sat as a sibling of HTTP Methods on the
  ViewDefinition run page while the same content nests beneath it on the
  SQLQuery run page. Nesting it removes the last instance of the
  structural inconsistency this branch set out to fix.
owen1uphealth

This comment was marked as outdated.

@johngrimes

Copy link
Copy Markdown
Collaborator Author

@owen1uphealth Apologies if I pulled the rug from under you by updating this PR. I would be grateful for any assistance you can provide in reviewing what we have here.

@oloveluck

Copy link
Copy Markdown

@johngrimes no worries, my harness auto-reviewed the PR when I pulled in the most recent changes, so I removed my comment.

The three decisions the PR descriptions make sense, and all my tests still pass after pulling in the changes. I plan on continuing to follow development here and will be able to provide more assistance as I become more familiar with the details in the existing spec.

@niquola

niquola commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

I fill something wrong about tableSource name, but merging to not block progress!

…eters

# Conflicts:
#	input/fsh/operations.fsh
#	input/pagecontent/OperationDefinition-SQLQueryExport-intro.md
#	input/pagecontent/OperationDefinition-SQLQueryExport-notes.md
#	input/pagecontent/OperationDefinition-SQLQueryRun-intro.md
#	input/pagecontent/OperationDefinition-SQLQueryRun-notes.md
@niquola

niquola commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Merged main into this branch to clear the conflicts introduced by #385 landing this morning. Ordinary merge commit (3d598c4), no rebase, so nothing in your history was rewritten.

How the conflicts were resolved. Five files conflicted: operations.fsh and the four SQLQuery pages. The merge base is c1da9a8 (#381), and everything main added to those files since then comes from #385 alone — #386, #387 and #381 were already on this branch. Since this PR rewrites all six files #385 touched, I took this branch's side throughout. The resulting tree is identical to what the branch had before the merge.

One thing that did not conflict, and should have. operations-common.md auto-merged, keeping both section: ## ViewDefinition table sources {#table-sources} from this PR and ## Table-Source Dependencies (view) {#table-source-dependencies} from #385, describing the same mechanism under two parameter names. Git saw no conflict because they landed in different parts of the file. I removed the #385 one — its seven rules are all covered by the matching algorithm, error table and precedence rule here.

Nothing from #385 was lost. Checked element by element: tableSource on both SQLQuery operations at 0..* with system, type and instance scope and targetProfile on both ViewDefinition and SQLView (so SQLView as a table source survives); version-aware matching and the client-supplied precedence rule; all four error conditions (three 400, one 404); the "supplied resources produce no output entries" rule on export. The worked examples are a superset — four here against one in #385.

Verification. SUSHI compiles with 0 errors. Full IG build: 37 errors, 18 warnings, 0 broken links across 83,923 links — the same pre-existing baseline you documented in #393, so this merge introduces no regression. Incidentally it also removes the six sql-on-fhir.org/ig/... canonicals that came in with #385's example; input/pagecontent is now back to zero of those after #387.

One open point for today's call: the parameter has now been renamed twice in two weeks (dependencyview in #385, viewResourcetableSource here), and the group has not ratified either. I have no objection to tableSource on the merits — it avoids the collision with the subject parameter on $viewdefinition-run and does not misdescribe a SQLView — but it should be a group decision rather than a merge artefact.

@johngrimes

Copy link
Copy Markdown
Collaborator Author

The reason I updated the proposal to tableSource was because I noticed that we already have the view parameter in the viewdefinition-run and viewdefinition-export operations with a different meaning...

Two system-level operations replace $viewdefinition-run,
$viewdefinition-export, $sqlquery-run and $sqlquery-export. Each acts on a
subject that may be a ViewDefinition, a SQLQuery or a SQLView. $sql-export
takes a repeating complex subject parameter so a mixed set of artefacts is
exported as one job; $sql-run takes one subject as flat primitives so GET
survives. Artefacts the server cannot resolve are supplied job-wide through
context, replacing tableSource.
Carries across the statement that file delivery is unconstrained and any
storage product named is informative, and the guidance to validate every
subject before starting a job rather than failing on the first.
Written by the IG Publisher, which the file's own header says must be
committed when resources are added or their id changes. The four superseded
entries are retained so their OIDs are never reused.
@johngrimes johngrimes changed the title Align parameters across the four data operations Collapse the four data operations into $sql-run and $sql-export Aug 4, 2026
@johngrimes

Copy link
Copy Markdown
Collaborator Author

Heads-up that this PR has changed substantially since it was last reviewed. It no longer only aligns the four data operations with one another - it replaces them with two, $sql-run and $sql-export, both invoked at the system level, each acting on a subject that may be a ViewDefinition, a SQLQuery Library or a SQLView Library. That follows the argument in #394, which the alignment work itself surfaced. The title and description have been rewritten accordingly.

@niquola your approval predates the collapse, so please treat it as stale - the contract it was given against no longer exists. Re-review requested when you have a moment.

@oloveluck this is a breaking change for your implementation. The operation codes, the subject parameter names, the dependency parameter name and the invocation levels have all changed since the commits you reported passing tests against. There is a parameter mapping table in the description covering each old operation to the new pair; the short version is viewCanonical/queryCanonical and their siblings become subjectCanonical/subjectReference/subjectResource, tableSource becomes a job-wide context, the repeating view and query parameters become one repeating subject, and instance- and type-level invocation are gone. Apologies for the churn - the reasoning for doing it now rather than after the ballot is in the description, and it is a decision worth contesting if you disagree.

The collapse commits are separable from the alignment commits that precede them, so it is possible to take the alignment alone if the collapse does not carry consensus. Three normative choices are called out in the description for review: system-level-only invocation, the name context (which collides with CQL $evaluate and CDS Hooks usage), and the single-snapshot requirement being a SHALL.

states no ordering, it is the only way a client correlates an entry with the
subject it requested. The value is determined in three steps:

1. If a `name` was supplied in that `subject` repetition, the server SHOULD use it

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If using the subject.name is only a SHOULD and the manifest carries no other key back to the subject, can a client rely on correlating inputs to outputs? Is there a reason why using the supplied name isn't a SHALL?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I've created an issue: #398

@oloveluck

Copy link
Copy Markdown

@johngrimes I pulled in the new OperationDefinitions, all tests pass, didn't find any obvious issues from the update.

@niquola
niquola merged commit 948d2b7 into main Aug 6, 2026
1 check failed
@johngrimes

Copy link
Copy Markdown
Collaborator Author

Thanks for reviewing @oloveluck!

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

Labels

None yet

Projects

None yet

4 participants