accept legal aggregate and property-path forms below the parser - #1629
Conversation
SPARQL 1.1 §19.8 reaches a path object through GraphNodePath, which admits RDFLiteral / NumericLiteral / BooleanLiteral, so `?s ex:ofMaker/ex:name "Acme"` is an ordinary matchable shape. Lowering forced every path object through Ref::try_from and rejected all of them with err:db/InvalidQuery. Carry the endpoint as a Term (plus the annotation surface's datatype constraint) and narrow per-arm instead of up front. Sequence final hops and alternative branches place it straight into the TriplePattern object slot. A simple inverse onto a literal lowers to a statically-empty pattern — RDF has no literal subjects, so that is zero solutions, not an error. Transitive arms (+/*/?) and negated property sets keep a narrow error naming the sub-case: their IR endpoints are typed Ref. Those shapes are legal SPARQL, so the message says the limitation is ours. Tested end-to-end, not by the W3C syntax suite: evaluate_positive_syntax_test never calls lower_sparql, which is why syn-pp-in-collection stayed green the whole time this was broken.
SPARQL 1.1 §18.5.1.1 counts an aggregate's argument within each group and does not exclude an expression that also appears in GROUP BY, so `SELECT ?k (COUNT(?k) AS ?n) … GROUP BY ?k` is legal. It returned HTTP 400, and so did HAVING (COUNT(?k) …) and COUNT(DISTINCT ?k) over the key — the reported "add DISTINCT" workaround never worked. The check was guarding something real: on the traditional grouping path GroupByOperator writes the scalar key into key columns and AggregateFn::apply returns a non-Grouped input unchanged, so the aggregate would have answered with the key term itself. Rather than delete the guard, make the hazard unreachable — copy each aggregated key into a fresh non-key column before grouping and point the aggregate at the copy. Expression::Var(k) preserves the key's unbound-ness, so an OPTIONAL-bound key keeps COUNT(?k) at 0 where COUNT(*) counts rows. The streaming path reads the copy as an ordinary column, so one rewrite serves both operators. variable_deps is computed from the pre-rewrite IR, so it is extended with the copies; otherwise GroupByOperator's projection trimming drops the column the aggregate now reads. The traditional path is pinned explicitly (GROUP_CONCAT forces it, EXPLAIN asserts which operators ran) — the streaming path alone would have passed even with the old hazard.
SPARQL 1.1 §18.5.1.1 defines COUNT(DISTINCT *) as the count of distinct
solutions in a group, and the W3C agg-count-rows-distinct test exercises it.
The parser accepted it and lowering returned not_implemented, so it was a 400.
The comment justifying that ("DISTINCT * is not meaningful for COUNT") was
wrong as a matter of spec and is corrected.
Adds a CountDistinctAll aggregate variant. It is the only aggregate that reads
the whole row rather than one column, so both group operators grew whole-row
plumbing: the streaming side keeps a HashSet over the composed, normalized row
(mirroring CountDistinct's per-value state), and the traditional side
reconstructs a group's solutions by zipping the Grouped columns of the
GroupByOperator output row.
Because it reports no input variable, the backward dependency walk cannot see
that it depends on every WHERE column — projection trimming would drop the
column that makes two solutions differ and undercount. compute_variable_deps
therefore disables trimming when it is present, and duplicate_insensitive is
false for the same reason (the dedup it gates also projects away columns).
Removes agg-count-rows-distinct from SPARQL11_AGGREGATES; the register's
stale-entry check enforces that the two stay in step. Aggregates suite is now
45 passed / 0 failed / 1 registered (was 44 / 0 / 2); the remaining entry is
the documented empty-named-graph divergence.
Known narrow divergence: the composed row is the whole pre-grouping batch, so
lowering-internal variables (property-path joins ?__ppN, blank-node variables)
participate in distinctness even though SPARQL projects them out of the
solution. Mainstream shapes are unaffected.
`*` denotes the solution mapping, and SPARQL projects the lowerer's synthetic variables out of it — property-path join variables (?__ppN) and non-distinguished blank-node variables (§4.1.4). Composing distinctness over the raw executor row let those split solutions the spec considers identical: two ex:a/ex:b routes between the same endpoints counted as 2, not 1. Silent overcount, no error. CountDistinctAll now carries the user-visible variable list, produced by the same filter SELECT * uses (extracted as `user_visible_vars` so the two cannot drift). Both group operators resolve it against their input schema and read only those columns; variables from other scopes and SELECT aliases never appear there and drop out in that intersection. The WHERE clause is lowered before the solution modifiers, so the registry is complete when the aggregate lowers. Regression tests cover a property-path join variable and a blank-node variable, on both the streaming and traditional grouping paths. Each asserts COUNT(*) alongside, so the row multiplicity is proven to exist rather than assumed.
agg_group_concat extracted strings only from Binding::Lit, so the three IRI representations (Sid / IriMatch / Iri) fell through its catch-all and were skipped. An all-IRI group concatenated nothing and returned Unbound — rendered null — and a mixed IRI/literal group silently dropped its IRI members while returning a plausible-looking string for the rest. Neither raised an error. Expand IRI bindings to their full IRI string in apply_aggregate, the wrapper that already pre-decodes values for exactly this aggregate family. The namespace table comes from ctx.active_snapshot, the same source STR() uses, so GROUP_CONCAT(?s) and GROUP_CONCAT(STR(?s)) now agree — the explicit STR workaround is redundant rather than required. Both lanes are covered: after the graph-view materialization (indexed ledgers, where an encoded ref decodes to a Sid) and without one (memory ledgers, where bindings arrive decoded). A Sid whose namespace code is missing from the table still cannot be expanded; it is left alone and skipped downstream, as before. Note SPARQL 1.1 §18.5.1.7 does not state a coercion for non-literal group members. Concatenating the IRI matches Jena and this engine's own STR(); the alternative reading is a type error. Worth confirming, but either is better than silently dropping members.
test_aggregate_on_group_by_key_errors pinned the blanket rejection this branch removes. Split it: COUNT over a key into a fresh output var now executes and returns the group's row count, while writing the output onto the key itself still errors — the key column stays in the schema, so the alias collides. The error text asserted is the schema-collision message rather than the removed rejection.
bplatz
left a comment
bplatz
left a comment
There was a problem hiding this comment.
CI is green now. Two inline comments.
On the GROUP_CONCAT question in the description: concatenating is the right call over erroring — Jena parity beats a type error for a shape that silently dropped members before.
Checked and clear, so nobody re-derives: never_matches is safe with a constant subject (apply_values never elides, and ValuesOperator with zero rows yields zero rows regardless of schema); and dropping dtc on the plain path surface really does match the hand-written triple, since term.rs:67 also lowers objects without a constraint.
|
|
||
| let num_cols = self.in_schema.len(); | ||
| let mut output_columns: Vec<Vec<Binding>> = Vec::with_capacity(num_cols); | ||
| // Namespace table for expanding IRI-valued GROUP_CONCAT members. |
There was a problem hiding this comment.
ctx.active_snapshot is one snapshot. In a dataset / multi-ledger query, is this the right namespace table for every IRI that reaches the aggregate?
If bindings arrive already re-encoded into a union namespace space, this is fine. If any arrive carrying a non-active ledger's namespace codes, they expand against the wrong table — which is the #1259 failure mode, and would show up as GROUP_CONCAT emitting a wrong IRI rather than dropping one. Neither the new tests nor the existing ones cover GROUP_CONCAT over a multi-ledger dataset, so the answer isn't pinned either way.
There was a problem hiding this comment.
fluree-db-query/src/aggregate.rs:228 — is ctx.active_snapshot the right namespace table?
You were right that nothing pinned this either way, and it was worth asking. The short version: for the dataset path it is correct, for a reason worth writing down, and there's now a test that fails if that ever stops being true. But the SERVICE path is a different story — see the bottom of this note.
Why the dataset path is safe. DatasetOperator stamps every cross-ledger binding before it leaves the member. dataset_operator.rs:388 stamp_provenance runs each batch through stamp_binding (:428), which converts Binding::Sid → Binding::IriMatch carrying the IRI decoded in its own ledger via ctx.decode_sid_in_ledger(sid, ledger_id) — and it errors rather than falling back to Binding::Sid, precisely because multi-ledger equality is defined around IriMatch. It's gated on needs_provenance, set to multi_ledger at :569, and :616-620 disables the binary store for those members so scans yield Binding::Sid rather than an EncodedSid that couldn't be decoded later. So by the time a batch reaches the aggregate, every cross-ledger IRI is an IriMatch, and group_concat_expand_iris reads that canonical iri field directly and never consults active_snapshot for it. The single-ledger case is trivially fine because active_snapshot is that ledger.
And it's pinned now (5f7abcaab). The new test seeds two ledgers under different prefixes — http://alpha.example/ and http://beta.example/ — that get allocated the same namespace code, since each ledger numbers its namespaces independently and in insertion order. That's what makes a wrong-table expansion visible rather than merely possible: expanding beta's SID against alpha's table produces http://alpha.example/b1, well-formed and wrong, which is exactly the #1259 shape you flagged.
I didn't want to ship a green test that was green for the wrong reason, so I mutation-checked it: routing the IriMatch arm through the namespace table instead of its canonical IRI makes it fail with
each subject must expand against its OWN ledger's namespace table,
got "http://alpha.example/a1|http://alpha.example/b1"
so the assertion really is load-bearing.
The part that isn't fine — and I don't think it's this PR's. While chasing the above I probed SERVICE against another ledger, and it does not go through stamp_provenance: service.rs:189-194 builds the inner tree against ctx.with_graph_ref(gref) and buffers the resulting rows as-is, so a Binding::Sid carrying the target ledger's namespace codes flows back out into a context whose active_snapshot is the parent. Against the same two-ledger fixture:
SELECT ?s WHERE { SERVICE <fluree:ledger:svc-beta:main> { ?s ?p "shared" } }returns http://alpha.example/b1 — the wrong IRI, with no aggregate anywhere in the query. GROUP_CONCAT(?s) over the same pattern inherits it, but the plain SELECT is the proof that this predates the change: the formatter is decoding a foreign SID against the primary snapshot. Two useful contrasts from the same probe — GROUP_CONCAT(STR(?s)) returns http://beta.example/b1 correctly, because eval_str runs per-row while the ctx is still the SERVICE target's; and the GRAPH <svc-beta:main> form is also correct, because that one does go through the dataset operator.
I'd rather not fix that here, and not because of appetite — it's that the obvious fix (stamp the SERVICE operator's returned batches the way dataset members are stamped) turns every SERVICE-returned IRI into an IriMatch, which changes join, DISTINCT, and GROUP BY key identity for every SERVICE query, and stamp_binding hard-errors on EncodedSid while the SERVICE path doesn't disable the binary store. That's a real change with its own test surface, sitting in a different subsystem from anything this PR touches. So my instinct is a follow-up issue with the repro above, and I'm happy to pick it up as the next thing — but if you'd rather it ride along here, say so and I will.
| .flat_map(Grouping::aggregates) | ||
| .any(|spec| matches!(spec.function, AggregateFn::CountDistinctAll(_))) | ||
| { | ||
| return None; |
There was a problem hiding this comment.
This disables dead-column trimming for the entire query whenever any COUNT(DISTINCT *) appears anywhere in it — including scopes the aggregate never reads, and including the common shape where it sits beside other aggregates over a wide WHERE.
The conservatism is correct; the blast radius is wider than the hazard. Worth either scoping it to the grouping's own input scope, or noting in the comment that it's deliberately query-global so the cost isn't a surprise later.
There was a problem hiding this comment.
fluree-db-query/src/execute/dependency.rs:49 — trimming disabled query-global
Agreed, and thank you — the blast radius really was wider than the hazard, and poking at your either/or turned up a third option that I think is better than either.
The thing I'd missed is that this aggregate does have inputs; it carries them. CountDistinctAll was given the user-visible variable list in ee670ad06 (the commit that stopped ?__ppN and blank-node variables from splitting solutions), so the backward walk doesn't need to be told "give up" — it needs to be told what to trace. 6d7d5b7a3 deletes the early return and extends the aggregate step of the walk instead, so CountDistinctAll's variable list is traced exactly the way every other aggregate's single input_var already is:
match &spec.function {
AggregateFn::CountDistinctAll(vars) => deps.extend(vars.iter().copied()),
other => deps.extend(other.input_var()),
}That lands better than either of the options you offered, I think. The pre-grouping stages keep precisely the columns the whole-row read needs and genuinely dead columns stay trimmable; the post-grouping sets are recorded earlier in the walk so they're untouched entirely; and variables the list names from other scopes simply never match a schema and drop out in compute_trimmed_vars. Net effect is that the common shape you named — COUNT(DISTINCT *) sitting beside other aggregates over a wide WHERE — now keeps its trimming everywhere except the columns that are actually load-bearing.
Same anti-vacuity check as above, since a dependency-set widening is the kind of thing that can look right while doing nothing: dropping just that one match arm fails five of the COUNT(DISTINCT *) tests, including sparql_count_distinct_star_survives_projection_trimming, which projects neither WHERE variable and still has to count 2. So the trace is doing real work rather than belt-and-braces.
bplatz
left a comment
bplatz
left a comment
There was a problem hiding this comment.
Left some feedback below to look at before merge
apply_aggregate expands IRIs with ctx.active_snapshot's namespace table, which is one ledger's. Nothing pinned whether that is right for a dataset query. It is, and for a reason worth recording: DatasetOperator stamps every cross-ledger Binding::Sid into Binding::IriMatch before the batch leaves the member (stamp_provenance), carrying the IRI already decoded in its OWN ledger and erroring rather than falling back. The expansion reads that canonical IRI and never re-expands a foreign SID. The test makes that machine-enforced. Its two ledgers use different prefixes that were allocated the same namespace code, so expanding one ledger's SID against the other's table yields a well-formed but wrong IRI. Confirmed discriminating by mutation: routing IriMatch through the namespace table instead of its canonical IRI produces http://alpha.example/b1 and the test fails.
Disabling projection trimming for the whole query was conservative in the right direction but far wider than the hazard: it also gave up trimming in stages the aggregate never reads, including the common shape where it sits beside other aggregates over a wide WHERE. The aggregate does have inputs — it carries them. Extend the backward walk's aggregate step to trace CountDistinctAll's user-visible variable list the way every other aggregate's single input_var is traced. The pre-grouping stages then keep exactly the columns the whole-row read needs, genuinely dead columns stay trimmable, and the post-grouping stages (recorded earlier in the walk) are untouched. Variables the list names from other scopes never match a schema and drop out. The trace is load-bearing, not belt-and-braces: dropping the arm fails five of the COUNT(DISTINCT *) tests, including the one that asserts a query projecting neither WHERE variable still counts 2.
|
Thank you, @bplatz — and thanks especially for chasing down Good to have the Both inline notes are addressed below. The Verification at Workspace-wide this time rather than the touched crates — that was the lesson from the two follow-up commits it took to get CI green here, and |
|
Following up on the Two things from chasing it that are worth having here, since both bear on what we concluded in this PR. The scope turned out narrower than I described. Joins across a And the representation was already mixed rather than uniform, which is the part that made this a small fix instead of a scary one. A cross-ledger Thanks again for pushing on the |
2 participants
Three cases where the parser accepts a legal SPARQL 1.1 form and a lower layer then rejects it with a 400, plus two aggregate-evaluation fixes found along the way.
Property paths with literal objects: lowering forced every path object through
Ref::try_from, so?s ex:ofMaker/ex:name "Acme"— an ordinary, matchable shape — was rejected wholesale. Endpoints now carry aTermand each arm narrows only as far as it must: sequence final hops and alternative branches take the literal directly, a simple inverse onto a literal lowers to a statically-empty pattern (RDF has no literal subjects — zero solutions, not an error), and the transitive arms keep a narrow error naming the sub-case, since their IR endpoints are typedRef.Aggregates over a
GROUP BYkey:SELECT ?k (COUNT(?k) AS ?n) … GROUP BY ?k(and theHAVING/COUNT(DISTINCT ?k)variants) returned 400. The check was guarding a real hazard on the traditional grouping path — the aggregate would have answered with the key term itself — so rather than delete it, each aggregated key is copied to a fresh non-key column before grouping and the aggregate reads the copy. The copy preserves unboundness, so anOPTIONAL-bound key keepsCOUNT(?k)at 0 whereCOUNT(*)counts rows. The traditional path is pinned explicitly via EXPLAIN (the streaming path alone would have passed even with the old hazard).COUNT(DISTINCT *)is now implemented (count of distinct solutions, §18.5.1.1 — the W3Cagg-count-rows-distincttest now passes and its register entry is removed), with distinctness scoped to user-visible variables so the lowerer's synthetic path-join and blank-node variables don't split solutions the spec considers identical.And
GROUP_CONCATover IRI-valued variables no longer silently drops them (it only extracted strings from literal bindings, so an all-IRI group returned null): IRIs expand through the same namespace tableSTR()uses, matching Jena. The spec states no coercion for non-literal members, so if we'd rather this be a type error than a concatenation, say so — either beats dropping members silently.