Skip to content

feat(graph-ir): fallible GraphSink protocol, quad capability, statement lifecycle + Turtle ParserOptions conformance - #1552

Open
aaj3f wants to merge 14 commits into
mainfrom
feat/graphsink-protocol
Open

feat(graph-ir): fallible GraphSink protocol, quad capability, statement lifecycle + Turtle ParserOptions conformance#1552
aaj3f wants to merge 14 commits into
mainfrom
feat/graphsink-protocol

Conversation

@aaj3f

@aaj3f aaj3f commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

PR-0: GraphSink protocol + Turtle ParserOptions

Foundation PR for the fluree rdf (riot-analog) effort. It lands nothing
user-visible: it changes the shared parser→sink abstraction so the writers,
converters and diagnostics of the later PRs are expressible at all, and it
makes the Turtle parser able to produce conformant RDF as well as the ingest
shape. Blast radius is every ingest path, so the gate is the full transact +
fluree-db-api suites, not the light crates alone.

Implements the locked decisions H-1 (PR-0 split), H-2 (ParserOptions on the
shared parser, ingest pinned to current behavior), and §1.2 of
riot-analog-pr-plan.md (fallible emission, finish(), quad capability
probe, term lifetime).

A. GraphSink protocol

Fallible emission. emit_triple / emit_list_item / emit_reified_triple
now return Result<(), SinkError>, and the new emit_quad does too. term_*
stay infallible — they intern a term and hand back an id; a writer only
touches its output on emit.

The point is early termination. Today a sink cannot report that its downstream
died, so convert big.ttl | head -5 would parse to EOF against a closed pipe.
SinkError carries an io::Error or a rejection message and answers
is_broken_pipe(), so a CLI can exit quietly on the expected shutdown and
loudly on a real failure. The Turtle parser stops at the first refusal and
preserves the sink's error as TurtleError::Sink rather than flattening it
into a generic parse error; the JSON-LD adapter does the same via
AdapterError::Sink.

Quad capability. supports_quads() + emit_quad(), mirroring the existing
supports_reified_triples probe. No producer emits quads in this PR — this is
the protocol landing, so M2's TriG/N-Quads readers have something to refuse
against.

The default emit_quad body errors rather than no-ops. The asymmetry with
emit_reified_triple (whose default stays a no-op) is deliberate: every
producer of reified-triple events already refuses the input up-front on the
capability probe, so that default body is unreachable, not a fallback. Nothing
guards quads yet, so the default has to be the guard. Silently folding named
graphs into the default graph is the exact defect the JSON-LD adapter has
today; this is what stops it being repeated.

Statement lifecycle. end_statement(), called by the Turtle parser at each
statement terminator and only for statements that parsed and emitted cleanly.
It serves two later features that need the same boundary: term lifetime (below)
and the statement-scoped output buffering --continue-on-error requires, since
triples are emitted during descent, before the terminating . proves the
statement valid. A failed statement never reaches the call, so it contributes
nothing.

finish() and the naming split. The trait gains
finish(&mut self) -> Result<(), SinkError> — flush and finalize, called by
whoever owns the sink, never by a producer (one sink can be fed by many
producer calls, a chunk each on the bulk-import path).

Three sinks already had an inherent finish() that consumed the sink and
returned its product. Leaving those named finish() would have type-checked
while letting a concrete-typed caller silently skip the protocol finish —
survivable for a graph collector, not for a writer. They are renamed to
consuming into_* methods, so finish() always means "flush" and never "give
me your contents":

sink was now
GraphCollectorSink finish() -> Graph into_graph()
FlakeSink finish() -> Result<Vec<Flake>, _> into_flakes()
ImportSink finish() -> Result<(writer, prefixes, spool), _> into_parts()

Deferred-error semantics are unchanged: FlakeSink/ImportSink emit_* still
record the first invariant violation and keep going, and the consuming method
is still where it becomes a hard error.

Term lifetime. TermIds now have a documented split. IRI and blank ids are
session-scoped (producers cache them across statements); literal ids are
statement-scoped, because literals are minted per occurrence and never
deduplicated — which is why the term table grew by one live entry per literal
occurrence for the length of the document.

GraphCollectorSink acts on it: end_statement retires the statement's
literal slots for reuse, so the high-water mark is the widest single statement
rather than the whole document (1000 statements × 2 literals leaves 4 slots,
not 2002). Soundness rests on two facts, both pinned by tests: emit_* clones
into the graph before returning, so overwriting a slot cannot reach an
already-collected triple; and the parser caches only IRI and prefixed-name ids
across statements.

This is groundwork, not a shipped win — no production path benefits today.
FlakeSink and ImportSink, the sinks on the ingest hot path, ignore
end_statement entirely and are deliberately untouched. GraphCollectorSink's
users are the W3C/SHACL testsuite harnesses, the R2RML loader, and
parse_to_json — none of them memory-bound. Producers that never delimit
statements, such as the JSON-LD adapter, never recycle at all and keep today's
allocation behavior exactly. The reason to land it here is that the memory
profile of a writer sink is the differentiator the effort is being built on,
and the protocol hook plus a proven-sound reference implementation is what a
writer needs to exist. Applying it to the ingest sinks is a separate,
separately-benchmarked change.

Statement rollback. abort_statement() is the counterpart to
end_statement(), and lands now so --continue-on-error does not force a
second breaking trait change later. The parser emits during descent — property
lists and collections push triples before the terminating . proves the
statement well-formed — so a rejected statement used to leave partial triples
in the sink; riot's "a rejected statement contributes nothing" held only in the
error return, not in the data. The parser calls it only when a statement fails
having already emitted, and exactly one of end_statement/abort_statement
ends any statement. GraphCollectorSink rewinds the graph to a mark taken at
the statement boundary.

Writing that test found a real bug in the first cut: the parser keeps one token
of lookahead, so consuming a statement's . lexes the first token of the
next statement, and a lexical error there was reported while the finished
statement was still in flight — the rollback then discarded a statement that
was complete and valid. Committing at the terminator, before the lookahead
advance, fixes it; the regression is pinned and mutation-verified.

Blank-node namespace. GraphCollectorSink minted anonymous blanks as
b{N}, the exact shape a document writes by hand, so a file containing _:b1
plus any anonymous node produced one node where RDF says two — silently merging
user data into a parser-minted node. Pre-existing via [ … ] and reifiers, but
CollectionStyle::Spine makes it systematic, since every collection mints
spine nodes. Fixed the way FlakeSink/ImportSink already do it: mint -b{N},
which cannot lex as a BLANK_NODE_LABEL and so is unreachable from any input.
The tests count distinct identities rather than checking isomorphism, because
merged nodes are isomorphic to themselves and an isomorphism check
structurally cannot see this class of bug.

B. Turtle ParserOptions

The parser canonicalizes in three places that are right for ingest and wrong
for conversion:

  • an object-position collection emits flat indexed list items, so the three
    triples W3C defines for ( ex:o ) arrive as one event;
  • an object-position () emits nothing — a silently lost triple, since
    () denotes rdf:nil;
  • Integer/Double tokens are parsed to native values, so +1, 01, 1e0,
    1.0E0 are unrepresentable (Decimal and i64-overflowing integers already
    keep their lexical form).

Meanwhile subject-position collections have always emitted a proper spine, so
the two positions disagreed with each other.

ParserOptions { collections, numerics } gets both shapes out of one grammar —
one conformance surface, no fork. ParserOptions::default() is today's
behavior in every field, so every existing caller is unchanged;
ParserOptions::conformant() is the faithful-RDF preset. New entry points
parse_with_options and parse_with_prefixes_base_options (the latter is the
full one — every other parse* delegates to it with something defaulted, so
the chunked reader paths can opt in too).

CollectionStyle::Spine needed no new collection code: it drops the
object-list special cases, so ( … ) and () fall through to the ordinary
object path, which already builds a spine and already resolves () to
rdf:nil. Both positions now share one code path.

One consequence worth naming for review: a spine collection has a single object
term (the head), so the RDF 1.2 annotation-on-collection deferral — which
exists only because indexed items leave nothing to reify — does not apply in
this mode. The deferral is unchanged in the default mode, and both halves are
pinned by a test.

NumericStyle::PreserveLexical routes numeric tokens through the typed-string
lane Decimal and IntegerOverflow already use. Deviation from the brief,
flagged for review:
the brief anticipated a new LiteralValue/Datatype
variant carrying the lexical string. That turned out to be unnecessary — the
existing typed-string lane already represents "this datatype, this exact
lexical", and is the pattern the brief also said to extend. So
fluree-graph-ir gains nothing for this feature, the ingest path hits the
identical branch it hits today, and there is no second numeric representation
to keep consistent. Downstream conversion is unaffected:
convert_string_literal already parses "42"^^xsd:integer back to a native
value (pinned by the pre-existing test_typed_string_integer).

Testing

New tests, beyond the existing suites staying green:

  • Early termination — a sink that refuses after N triples: the parse stops
    there (emits exactly N), the sink's error survives as TurtleError::Sink
    with is_broken_pipe() intact, and the failed statement is never terminated.
  • Lifecycleend_statement fires once per completed statement,
    directives included; a multi-triple predicate-object list is one statement;
    the parser never calls the protocol finish(), and two parses can feed one
    sink.
  • Quad refusal — triple-only sinks report supports_quads() == false, and
    the default emit_quad body returns an error instead of dropping the graph
    name.
  • Recycling — literal slots are reused after a statement ends, IRI and
    blank ids are not, an already-emitted triple is unaffected by the reuse, and
    the term table tracks the widest statement rather than the document; plus an
    end-to-end Turtle guard that a later statement reusing an earlier slot cannot
    corrupt the earlier values.
  • Collections — W3C-shaped spine assertions that walk the
    rdf:first/rdf:rest chain rather than counting triples; one-item,
    two-item, nested and nested-empty collections; () in both positions;
    subject and object positions agreeing; annotations admitted in spine mode and
    still refused in the default.
  • Numerics — seven lexical forms preserved with correct datatypes;
    canonicalization still the default; decimals and big integers identical under
    both modes; non-numeric literals untouched by the knob.
  • Default-mode pinning — one document covering every construct the options
    touch, asserted triple-for-triple in the default mode. This is the regression
    guard for "existing callers are unchanged". The pre-existing
    test_collection, test_empty_collection and test_integer_literal
    assertions are unmodified and still pass.

The recycling differential

Slot recycling is the one change here whose failure mode is aliasing: a wrong
answer is a perfectly well-formed graph with one term silently substituted for
another. Per-triple expectations written by the author of the recycling are
unlikely to anticipate that, so fluree-graph-turtle/tests/adversarial_recycling.rs
asserts against an oracle instead — the same document parsed into a recycling
sink and into one whose end_statement is swallowed by a forwarding wrapper,
i.e. today's behavior. Any TermId outliving its statement while still
referenced shows up as a divergence.

Two things make that evidence rather than ceremony:

  • Mutation-checked, with the limits stated. Recycling IRI slots — which the
    parser caches across statements — is detected by 2 of the 9 tests in the
    file through output comparison alone, and by 9 of 9 once the retirement
    assert is present
    ; the 7 additional detections come from the assert
    tripping on the polluted slot list, not from any graph diverging. Both
    figures were measured by applying the mutation with the assert enabled and
    disabled.
  • A second mutation, always reusing literal slot 0, escapes this file
    entirely and is caught only by the fluree-graph-ir unit tests. The reason
    is worth knowing: no literal is ever minted while another literal id is live
    (the parser emits each literal immediately after minting it), so
    slot-0-collapse is observationally equivalent here. The differential's power
    comes from the aliasing dimension, not from statement width. Both facts are
    recorded in the module docs so nobody trims the corpus believing the cases
    are redundant.
  • Corpus-scale. Run against the real W3C rdf-tests Turtle corpus —
    1148 files, 718 parsed, 430 rejected, across all three option modes
    (default, conformant, spine-only) — zero output divergence from the
    no-recycle oracle and zero acceptance divergence
    , with two temporary
    probes live in end_statement: a poison canary overwriting every retired
    slot with a sentinel, and an assertion that every retiring slot still holds a
    Literal. Neither probe ships; both are review instrumentation.

The corpus harness itself is not in this PR: it hardcodes an absolute path
into the rdf-tests submodule. Its home is the testsuite-rdf harness of the
plan's §2, which is where this differential should be re-landed so the result
keeps being enforced rather than being a one-off measurement.

Gates

  • cargo nextest run -p fluree-graph-ir -p fluree-graph-turtle -p fluree-graph-json-ld -p fluree-graph-format -p fluree-db-transact — 730 passed, 0 failed.
  • cargo nextest run -p fluree-db-api (full, grouped grp_* bins, no per-file
    filters) — 2919 passed, 0 failed, 11 skipped.
  • cargo clippy --all-targets --no-deps on every touched crate — clean
    (workspace deny-lints, incl. uninlined_format_args).
  • cargo fmt --all --check — clean.
  • cargo check --all-targets on the excluded testsuite-sparql and
    testsuite-shacl workspaces — clean. They call the renamed
    GraphCollectorSink::finish and would otherwise break in their own CI job,
    which the workspace gates do not cover.

Review follow-ups deferred to M1

Raised in review, agreed as non-blocking, recorded here so they are not lost:

  • Record that the two lifecycle guards do not overlap. The two mutations
    on committed_current are caught by disjoint sets of tests: dropping the
    guard (aborting a statement that already committed) is caught only by
    lexical_error_in_lookahead_does_not_abort_the_committed_statement, while
    dropping the per-statement reset is caught only by
    failed_statement_contributes_nothing and the two
    abort_still_fires_after_a_committed_statement_* cases. Neither side
    subsumes the other, and every one of them looks redundant beside its
    neighbours until you actually run the mutations. The test file currently
    explains the latch hazard but not this mapping; it wants one comment naming
    which test covers which mutation, so nobody trims either side.
  • Upstream the rest of the reviewer's probe. The two
    after-a-committed-statement rollback cases are already shipped, with
    different fixtures — the reviewer reaches the error-at-dot path with a
    trailing , where this PR uses a wrong token in the terminator position.
    What is not covered here is their
    exactly_one_terminator_per_statement_everywhere sweep, which asserts the
    invariant across bracket, collection, bare-bnode, subject-collection,
    SPARQL-prefix and nested-list shapes in a single pass. That sweep is the
    valuable remainder.
  • Promote the sharp invariant phrasing into the parser. The precise form
    is "no literal is minted while another literal id is live"; the comment at
    parse_annotation_tail still leads with the looser "at most one literal id
    is live at a time", which is not literally true (parse_reified_triple
    holds a literal object while minting a reifier — the reifier grammar just
    guarantees that term is never a literal). The sharp form is already in the
    test module docs; the parser comment should lead with it too.

Not in this PR

No quad producer, no writer sink, no CLI surface, no --continue-on-error
those are the protocol's first consumers, in M1/M2. FlakeSink/ImportSink do
not recycle literal slots and do not implement abort_statement; both are
hot-path changes that want their own benchmarking. The JSON-LD adapter has no
statement boundaries, so it neither recycles nor rolls back, and it still drops
@graph contents — its module docs now say so accurately (they previously
claimed "flattened into the default graph", which is not what the code does),
and fixing it needs the quad protocol this PR lands.

aaj3f added 14 commits July 28, 2026 20:06
…ycle

The sink protocol had no error channel: every emit method returned `()`, so
a sink that writes somewhere could not report failure and a producer could
not stop. `convert big.ttl | head -5` would parse to EOF against a dead
pipe. It also had no trait-level flush, and no way for a producer to tell a
sink when a statement ended.

Protocol changes (fluree-graph-ir/src/sink.rs):

- `emit_triple` / `emit_list_item` / `emit_reified_triple` return
  `Result<(), SinkError>`; `SinkError` carries an `io::Error` or a rejection
  message and answers `is_broken_pipe()` so a CLI can exit quietly on a
  closed downstream. `term_*` stay infallible — they only intern.
- `supports_quads()` + `emit_quad()`, mirroring the `supports_reified_triples`
  capability probe. The default body errors rather than no-ops: no producer
  emits quads yet, so this backstop is what stops a future one from silently
  folding graph names into the default graph. (`emit_reified_triple`'s
  default stays a no-op — every producer of those events already refuses
  up-front on the probe, so its default is unreachable, not a fallback.)
- `end_statement()`, called by the Turtle parser at each statement
  terminator, and only for statements that parsed and emitted cleanly. Two
  consumers: statement-scoped term lifetime (literal ids are per-occurrence
  and are the O(document) growth term), and the statement-scoped output
  buffering `--continue-on-error` needs, since triples are emitted during
  descent before the terminating `.` proves the statement valid.
- `finish()`, the protocol flush. Producers do NOT call it: one sink can be
  fed by many producer calls (a chunk each on the bulk-import path).

Because `finish()` now means "flush", the sinks that also produce something
expose that separately as a consuming method — `GraphCollectorSink::into_graph`,
`FlakeSink::into_flakes`, `ImportSink::into_parts`. Leaving them named
`finish()` would have let a concrete-typed caller silently skip the protocol
finish, which is exactly the trap a writer sink cannot survive.

Deferred-error semantics in FlakeSink/ImportSink are unchanged: `emit_*`
still records the first invariant violation and keeps going, and the
consuming method is still where it becomes a hard error.

The Turtle parser propagates sink errors immediately, and preserves them as
`TurtleError::Sink` rather than flattening them into a generic parse error.
The JSON-LD adapter does the same via `AdapterError::Sink`; its module docs
now say `@graph` contents are dropped rather than flattened, which is what
the code does.
`GraphCollectorSink` kept one live term-table entry per literal
*occurrence* for the length of the document: literals are minted per
occurrence and never deduplicated, unlike IRI and blank ids, which
producers cache and reuse. On a large Turtle file that means every literal
is held twice — once in the term table, once in the graph.

`end_statement` (landed with the protocol) marks the point where the
statement's literal ids are provably dead, so the sink now reuses their
slots. The high-water mark becomes the widest single statement instead of
the whole document; a 1000-statement document of two literals each leaves 4
term slots behind rather than 2002.

Soundness rests on two facts, both pinned by tests: `emit_*` clones a term
into the graph before returning, so overwriting a slot afterwards cannot
reach a triple already collected; and the Turtle parser caches only IRI and
prefixed-name ids across statements, never literal ids.

Producers that do not delimit statements — the JSON-LD adapter, and any
direct caller of the sink API — never call `end_statement` and so keep
today's allocation behavior exactly.
The parser canonicalizes in three places that are correct for ingest and
wrong for conversion:

- an object-position collection emits flat indexed list items, so the three
  triples W3C defines for `( ex:o )` arrive as one event;
- an object-position `()` emits nothing at all — a silently lost triple,
  since `()` denotes `rdf:nil`;
- `Integer` and `Double` tokens are parsed to native values, so `+1`, `01`,
  `1e0` and `1.0E0` are unrepresentable. (`Decimal` and i64-overflowing
  integers already keep their lexical form.)

Subject-position collections, meanwhile, have always emitted a proper
spine, so the two positions disagreed with each other.

`ParserOptions { collections, numerics }` gets both shapes out of one
grammar. `ParserOptions::default()` is exactly today's behavior in every
field, so every existing caller is untouched; `ParserOptions::conformant()`
is the faithful-RDF preset. New entry points `parse_with_options` and
`parse_with_prefixes_base_options`; the existing `parse` /
`parse_with_prefixes_base` now delegate with defaults.

`CollectionStyle::Spine` needs no new collection code: it drops the
object-list special cases so `( … )` and `()` fall through to the ordinary
object path, which already builds a spine and already resolves `()` to
`rdf:nil`. One code path now serves both positions. A consequence worth
naming: a spine collection has a single object term (the head), so the RDF
1.2 annotation-on-collection deferral — which exists only because indexed
items leave nothing to reify — does not apply in this mode.

`NumericStyle::PreserveLexical` routes numeric tokens through the
typed-string lane that `Decimal` and `IntegerOverflow` already use, rather
than adding a second representation to the IR. Nothing in fluree-graph-ir
changes, and the ingest path keeps hitting the identical branch it hits
today.

Tests ship with the feature: W3C-shaped spine assertions that walk the
rdf:first/rdf:rest chain rather than counting triples, empty collections in
both positions, the seven lexical forms, and — as the regression guard for
"existing callers are unchanged" — a triple-for-triple pin of default-mode
output across every construct the options touch. The pre-existing
`test_collection` / `test_empty_collection` / `test_integer_literal`
assertions are unmodified and still pass.
The recycling failure mode is aliasing, not arithmetic: a wrong answer is a
perfectly well-formed graph with one term silently substituted for another.
Per-triple expectations written by the author of the recycling are unlikely
to anticipate that, so this asserts against an oracle instead — the same
document parsed into a sink that recycles and into one whose
`end_statement` is swallowed by a forwarding wrapper, i.e. today's
behavior. Any TermId outliving its statement while still referenced diverges.

The corpus is chosen to reuse slots at every width: statements that emit
during descent (nested property lists, collections at two levels,
subject-position spines), literal counts that shrink and grow across
boundaries, repeated identical literals as aliasing bait, and blank-node
labels shaped like the anonymous mint. Run in default and spine modes, plus
the bulk-import shape of one sink fed by many parses.

Mutation-checked rather than assumed: recycling IRI slots — which the parser
caches across statements — fails both hostile-document cases. Notably the
single-sink and failed-statement cases do NOT catch that mutation, which is
recorded in the module docs so nobody trims the corpus believing they are
redundant.

The failed-statement case pins an observable protocol property rather than
recycling: a statement that errors after emitting never reaches
`end_statement`, and the sink keeps the half-statement's triples. There is
no rollback hook, which is precisely why `--continue-on-error` will need the
statement-scoped buffering `end_statement` exists to enable.

Approach and hostile corpus came from the adversarial review of this PR.
`GraphCollectorSink` minted anonymous blank nodes as `b{N}`, which is
exactly the shape a document writes by hand. A file containing `_:b1` and
any anonymous node therefore produced ONE node where RDF says two, silently
merging user data into a parser-minted node.

Reproduced: `_:b1 ex:p "user-one" .` plus a spine collection in one
document leaves `_:b1` owning three triples instead of one.

Pre-existing via `[ … ]` and reifiers, but `CollectionStyle::Spine` makes it
systematic — every collection mints spine nodes — so it has to be fixed
here rather than filed.

The fix is the one `FlakeSink` and `ImportSink` already carry, with the same
rationale recorded in their comments: mint `-b{N}`. Turtle's
BLANK_NODE_LABEL must start with PN_CHARS_U | [0-9], so `_:-b1` can never
lex and the minted namespace is unreachable from any input, while '-' stays
legal medially so the label still serializes.

Two tests, both mutation-verified against the `b{N}` mint: a unit test that
the minted label cannot lex as a user label, and a Turtle-level test that
`_:b1`/`_:b2` alongside a spine collection and a `[ … ]` keep every identity
distinct, in both option modes. Deliberately identity-counting rather than
isomorphism-based: merged nodes are isomorphic to themselves, so an
isomorphism check structurally cannot see this class of bug.
…thing

The parser emits during descent: property lists and collections push triples
before the terminating `.` proves the statement well-formed. A statement that
failed therefore left its partial triples in the sink, so riot's rule — a
rejected statement contributes nothing — held only in the error return, not
in the data. `--continue-on-error` cannot be built on that.

`abort_statement()` is the counterpart to `end_statement()`, landing now so
`--continue-on-error` does not force a second breaking trait change later.
Default no-op. The parser calls it only when a statement fails having already
emitted; exactly one of the two ends any statement.

`GraphCollectorSink` implements it by rewinding the graph to a mark taken at
the statement boundary and retiring the statement's literal slots, and
`Graph::truncate` is the primitive for that.

Writing the test found a real bug in the first cut of this. The parser keeps
one token of lookahead, so consuming a statement's `.` lexes the FIRST token
of the next statement. A lexical error there was reported while the finished
statement was still in flight, and the rollback then discarded a statement
that was complete and valid — `ex:a ex:p "1" ; ex:q "2" .` followed by a
garbage line lost every triple, not just the garbage. The fix is to commit at
the terminator, before the lookahead advance: `end_statement_at_dot` replaces
the bare `expect(Dot)` in `parse_triples` and in the dotted directives, and
`parse_statement` reports whether it already committed so the caller does not
commit twice. A statement that commits and then fails on the advance still
reaches `abort_statement`, but the rollback is a no-op because the commit
moved the mark past it.

Both behaviors are pinned and both tests are mutation-verified: moving the
commit back after the advance fails
`failure_before_any_emit_leaves_earlier_statements_intact`.
…t_quad

The two capability-gated events now behave identically: `debug_assert` in
development, `SinkError::Rejected` in release. A dropped reification is data
loss in exactly the way a dropped graph name is, so neither default should
be a silent no-op.

The asymmetry was defensible when written — every producer of reified-triple
events checks `supports_reified_triples` and rejects the input up-front, so
the default body is unreachable on any path that exists today — but
"unreachable" is a property of today's producers, not of the protocol, and
it costs nothing to make the backstop real.

Covered by a sink that claims the capability without overriding the method,
which is what reaches the default past its debug assert.
Two mutations, two different outcomes, both now written down where the next
person will look. Recycling IRI slots fails the hostile-document cases, so
those are load-bearing. Always reusing literal slot 0 escapes this
differential entirely and is caught only by the fluree-graph-ir unit tests —
because outside the annotation path the parser emits each literal
immediately after minting it, so at most one literal id is live at a time and
slot-0-collapse is observationally equivalent here.

Worth stating plainly rather than leaving as a passing test: this
differential's power is the aliasing dimension, not statement width. If a
future producer holds several literal ids at once that stops being true, and
this file needs a case for it.
`literal_slots` may only contain slots minted by `add_literal_term`. If a
session-scoped term ever lands in one, retiring it hands a producer-cached
id to the next literal — and the result is a graph that still looks
perfectly well-formed with one term substituted for another, which is the
hardest possible failure to notice downstream.

So check it where the retirement happens: every slot the statement is about
to retire must still hold a `Term::Literal`. Debug builds fail loudly and
name the offending slot and term; release builds compile the whole thing
out.

This is a strictly stronger guard than the differential. Routing `term_iri`
through the literal lane trips the assert in
`one_sink_many_parses_survives_recycling`, which the output differential
alone does NOT catch — the corruption is invisible there but the invariant
violation is not.

Applied at `abort_statement` too, which retires the same slots.
Everywhere else the parser emits a literal immediately after minting it, so
at most one literal id is live at a time — which is exactly why a sink can
recycle literal slots per statement without any live id being clobbered.

`parse_annotation_tail` is the one exception: `object` may be a literal id
and it stays live across the annotation body, which mints further terms
including literals of its own, before being handed to `emit_reified_triple`.

Unreachable today, because star constructs require
`supports_reified_triples()` and the only recycling sink returns false — so
no sink both recycles and accepts these events. It goes live the moment a
writer sink supports both, which is squarely on this effort's roadmap.

Recorded at the site rather than in a design doc, with the two ways out
(keep literal ids valid for the whole statement, which `end_statement`
already promises; or materialize `object` before parsing the body), because
whoever writes that sink will be reading this function, not the doc.
The disjointness test asserted over a document containing both a collection
and a `[ … ]`, which reads like coverage but is not: the spine assertion
fires first, so a regression confined to the bracket path could hide behind
it. The two sites also fail independently — `[ … ]` mints in EVERY mode,
while spine nodes only exist under `CollectionStyle::Spine` — so a
spine-only test would pass with the bracket path broken.

Now each site is asserted on its own document with an explicit distinct-node
count: Spine `( ex:a ex:b )` against `_:b1`/`_:b2` must leave 4 identities,
and default-mode `[ ex:q "anon" ]` against `_:b1` must leave 2. Both were
checked to fail on their own against the old `b{N}` mint — the bracket case
collapses 2 nodes to `["b1"]`.

Counting identities rather than checking isomorphism is the point: merged
nodes are isomorphic to themselves, so an isomorphism check cannot see this
class of bug at all.
The trait publishes "exactly one of end_statement/abort_statement per
statement". The parser broke it: the error arm gated only on whether the
statement had emitted, while the fact that it had already committed rode
back on `parse_statement`'s return value — which the error path discards. A
statement that committed at its `.` and then hit a lexical error in the
one-token lookahead therefore logged End followed by Abort.

Benign today (the commit had already moved GraphCollectorSink's mark, so the
rollback was a no-op) and not benign for the buffering sink the hook exists
for, which would see a commit revoked.

Fixed by moving the answer onto `self` as `committed_current`, which is where
it has to live because the case that needs it is the error path. That also
removes the redundant `Result<bool>` plumbing through `parse_statement` and
both directive parsers — one source of truth, consulted by both arms.

Mutation-verified: dropping the `!self.committed_current` guard reproduces
the `[End, Emit, Emit, End, Abort]` sequence exactly, and only the new
lookahead test catches it.
"Exactly one of end_statement/abort_statement per statement" is a claim
about the event sequence, and the previous tests only checked the resulting
graph — which is why a spurious Abort survived them: it was a no-op for a
collector sink and would only corrupt a buffering one. A `LoggingSink` now
records the order and the tests assert it.

The old comment was also wrong about its own fixture. It claimed `@@@` fails
"at its subject before a single emit"; `@@@` is a LEXICAL error raised while
advancing past the PREVIOUS statement's terminator, so it never reaches
subject parsing at all. That one document was standing in for two different
guards.

Split accordingly, one guard each:
- `failure_before_any_emit_does_not_abort` uses `"strsubject"` as a subject,
  which lexes fine and is rejected by `parse_subject` — the only way to fail
  inside a statement with zero emissions, which is the emit-count guard's
  true branch.
- `lexical_error_in_lookahead_does_not_abort_the_committed_statement` keeps
  `@@@`, now correctly described, covering the committed-statement guard.
- `failed_statement_contributes_nothing` gains the full expected sequence.

Also sharpens the recorded invariant behind the slot-0 mutation gap: it is
"no literal is minted while another literal id is live", not "one term id at
a time". `parse_reified_triple` does hold a possibly-literal object while
minting the reifier, but the reifier grammar is `iri | BlankNode`, so no
literal is minted there.
The flag's specific failure mode is latching. It is reset at each statement
start; if that reset were ever lost it would stay true after the first
committed statement and `abort_statement` would never fire again — rollback
dying silently, which is strictly worse than the contract bug the flag fixes,
because nothing would look wrong.

Two guards, both a committed statement followed by one that emits and then
fails, so the trailing Abort is mandatory:
- a wrong token where the `.` belongs (`ex:q` lexes fine, so the triple is
  emitted and the statement fails at its terminator);
- a missing final dot at EOF, the shape a truncated file produces.
Both assert ["End","Emit","End","Emit","Abort"] and one surviving triple.
Mutation-verified: removing the reset drops the trailing Abort from both, and
from `failed_statement_contributes_nothing`, which is why that test was left
alone — it doubles as a latch guard.

Also records a case that looks like the wrong-terminator one and is not:
`ex:b ex:p "2" @@@` fails BEFORE emitting, because a literal's `@lang`/`^^type`
suffix is checked by lexing the next token, so the lexer dies inside the
suffix lookahead. No emit, so no Abort — logged as ["End","Emit","End"].
Worth pinning precisely because the two differ only in the failing token.
@aaj3f

aaj3f commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

CI note: the test job failure is the known flake #1489 (binary-index reload race in values_bound_type_after_delete_incremental_index_timeline under parallel grp_misc — filed 2026-07-13, pre-dates this branch). The test passes in 2.6s on a local merge of this head (8697030) with current main (5598ffd). Rerunning the job.

@bplatz bplatz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving as-is, no changes requested — the protocol design is sound and the adversarial recycling suite is unusually good.

One thing to settle before M1, not in this PR: I think the tool should be fluree convert, not fluree rdf.

  • fluree export already writes Turtle / N-Triples / N-Quads / TriG / JSON-LD, so "RDF" is not what distinguishes the new tool. The real axis is that it is file → file with no ledger in the loop — which rdf does not say, and which export will keep shadowing in a user's head.
  • Our data-flow commands are verbs (export, load, insert, query, validate); the nouns are reserved for management groups with sub-verbs under them (graph, branch, remote). A format converter is a data-flow operation, so a top-level rdf noun sits against that grain.
  • CSV already reaches this same GraphSink path via csv_import.rs → JSON-LD, and we import Cypher dumps too. A reader for either lands next to this machinery sooner or later, and fluree rdf convert --from csv argues with itself.
  • convert costs nothing if the scope stays RDF-only and needs no rename if it does not. A --help line pointing at riot / rapper / rdfpipe keeps the legibility either way.

Nothing here blocks this PR — it ships no CLI surface, which is exactly why now is the cheapest moment to settle the name.

@aaj3f

aaj3f commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for this — and agreed that now, pre-merge, is the right moment to settle it. Two of your points land cleanly: export already writes all of these formats, so "RDF" is genuinely not the differentiator (file→file with no ledger in the loop is), and the --help pointer at riot/rapper/rdfpipe is a good idea we'll take regardless of the name.

The complexity that I think changes the calculus: convert is the flagship verb but not the scope. The locked plan's later milestone adds check, count, canon, infer, and a file-mode query as siblings — a riot/rapper-class toolkit, not a single converter. Under the top-level-verb grammar those become fluree check (directly beside the existing fluree validate, which is SHACL-against-ledger — two adjacent top-level verbs both meaning "tell me if this is OK" with different targets), fluree count, fluree canon. The naming problem doesn't go away with the rename; it comes back one milestone later, worse.

Which is also why I'd read your own grammar observation as supporting the group form: the nouns in our CLI are groups-with-sub-verbs (branch, graph, remote), and rdf is exactly that — a tool group whose sub-verbs are data-flow operations on files. The scope line also answers the CSV case: rdf scopes to RDF syntaxes, and CSV→RDF is a mapping decision (header→predicate, R2RML territory), not a syntax conversion — so it lands next to import/the mapping machinery, and fluree rdf convert --from csv never needs to exist.

One correction to the cost model, in fairness to both sides: the CLI surface does already exist in the stacked PRs above this one (#1555/#1569/#1578 ship fluree rdf convert — command registration, ~90 test invocations, docs, and the bench harness's recorded invocations). Nothing is merged, so a rename is still a mechanical sweep rather than a breaking change — but it's "one focused sweep across three open PRs," not "costs nothing."

The trade-offs as I see them:

  • Keep fluree rdf <verb> — one canonical name, the M5 verbs have a home, and fluree rdf reads as "the RDF toolkit" to the exact audience we're courting away from riot. Cost: convert alone sits noun-side of your grammar.
  • Rename to fluree convert — best verb grammar for the flagship. Cost: the sweep, plus re-solving check/canon/count placement next milestone (top-level collides with validate; a convert-anchored group has no name for them).
  • Middle path: keep the group, add top-level fluree convert as a visible alias for rdf convert — your discoverability point conceded for the one verb users will actually reach for first, at ~30 lines and no test churn. Cost: two spellings of one command in docs and support.

Our lean: keep fluree rdf <verb> as canonical for the toolkit-scope and collision reasons above — and if you think the verb-grammar/discoverability case for the flagship is strong enough, we'd happily add the top-level convert alias at M1 as the settlement. Genuinely open on that last part; it's cheap and it's your point that motivates it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants