Skip to content

FC-837 remove deprecation syntax warnings - #5

Merged
ldw1007 merged 1 commit into
masterfrom
fix/remove-deprecation-notices
Oct 23, 2020
Merged

ldw1007 merged 1 commit into
masterfrom
fix/remove-deprecation-notices

Conversation

@ldw1007

@ldw1007 ldw1007 commented Oct 22, 2020

Copy link
Copy Markdown
Contributor

removed warnings about "deprecated" syntax for analytical queries

removed warnings about "deprecated" syntax for analytical queries
@bplatz
bplatz self-requested a review October 23, 2020 01:07
@ldw1007
ldw1007 merged commit 5a667e5 into master Oct 23, 2020
@ldw1007
ldw1007 deleted the fix/remove-deprecation-notices branch October 23, 2020 12:27
@aaj3f aaj3f mentioned this pull request Apr 30, 2026
5 tasks done
bplatz added a commit that referenced this pull request May 17, 2026
… diagnostics, nits

Selected items from the additional review feedback. Each call
explained.

**#1 — Retry-fragility comment on `take(opts.shapes)`.** No retry
exists on the staging path today, but `take()` moves the inline
SHACL JSON off the txn, so a future retry would silently skip
inline-shapes validation on the second attempt. Hard invariant
comment added at the move site; if retry is ever added, defer
the take until after `stage_txn` returns successfully.

**#3 — `UnsupportedFeature` errors no longer carry an empty
`ledger_id`.** `resolve_graph_ref` rejected `f:atT` /
`f:trustPolicy` / `f:rollbackGuard` *before* verifying that the
ref was actually cross-ledger, so a misrouted same-ledger ref
hitting this path would have produced a confusing error with
`ledger_id: ""`. Reordered: the same-ledger-ref guard runs first,
binding `raw_ledger_ref` once; the unsupported-feature checks
now carry the real ledger id in every diagnostic.

**#4 — `f:atT` rejection breadth: audited, no gap.** All five
`f:GraphRef`-shaped predicates already reject `f:atT` at their
same-ledger entry points:
- `policy_builder::resolve_policy_source_g_ids`
- `tx::resolve_shapes_source_g_ids` (SHACL)
- `ontology_imports::resolve_schema_bundle`
- `tx::resolve_constraint_source_g_ids_for`
- `view::fluree_ext::resolve_local_rules_source_g_id`

Plus the cross-ledger `resolver` (now with non-empty ledger_id
per #3). Audit-only change; no code modification beyond the doc
note.

**#2 — Cross-ledger policy target IRI loss is now logged.**
`wire_to_restrictions` previously `filter_map`'d unresolvable
target IRIs and `for_classes` IRIs silently. A cross-ledger
policy could quietly narrow its scope on D if M referenced IRIs
D had never registered. Two `tracing::warn!` calls now surface
both cases with `restriction_id`, requested count, and resolved
count — operators can spot the silent narrowing in logs.

Kept the restriction even when targets become empty (rather than
dropping the whole restriction): same-ledger via
`load_policy_restriction` also produces empty target sets for
the same IRI-not-in-snapshot scenario, and dropping the
restriction would diverge cross-ledger from same-ledger
semantics. The warn log is the parity-preserving signal.
`fluree-db-policy` gains a `tracing` dependency for this.

**Nits taken:**
- `TAtUnavailable` doc-commented as Phase 3 reserved (unreachable
  today because `UnsupportedFeature { feature: "f:atT" }` fires
  first).
- Empty `uniqueProperties: []` over HTTP is now documented as an
  intentional "no inline constraints" treatment, not a silent
  bug.
- Cache read/write race in `resolver` documented as benign:
  losers materialize the same value structurally; single-flight
  is a future optimization, not a correctness need.
- `f:AccessPolicy` IRI promoted from local `const` /
  duplicated-literal to `fluree_vocab::policy_iris::ACCESS_POLICY`.
  Two call sites updated; nothing else used the local consts.

**Nits skipped per "don't make changes that don't seem needed":**
- HTTP-level negative test for malformed `opts.shapes` /
  `uniqueProperties` (#5) — unit-level coverage exists; HTTP
  harness setup cost not justified.
- Same-ledger `rules_source_g_id` end-to-end test (#6) — already
  covered by `it_rules_source::rules_source_in_named_graph_is_honored`,
  which runs an actual query against the named-graph rule.
- DynamoDB hard-drop failure handling (#7) — storage layer,
  unrelated to this PR.
- Duplicate pattern match in `shapes_materializer` —
  cosmetic.
- `snapshot.ledger_id` redundancy in inline-shapes / inline-
  ontology `txn_id` — cosmetic.
- `is_last_live_branch` WARN log — unrelated path.
bplatz added a commit that referenced this pull request Jun 3, 2026
…rror

SPARQL 1.1 §11.4 (GROUP BY) / §18.5: grouping or projecting a variable that the
pattern never binds is legal — the variable is unbound, so every solution shares
that value and they collapse into a single group on it. Fluree instead failed at
plan time ("GROUP BY variable not found in query schema" / "Selected variable not
found in query schema").

In the shared solution-modifier tail, before grouping, materialize any GROUP BY
key — or, in an ungrouped query, any SELECT variable — that is absent from the
WHERE output as an Unbound column (an identity `BIND(?v AS ?v)` over the absent
variable evaluates to Unbound). Grouping and projection then treat it as unbound
rather than failing the schema lookup. Aggregate INPUT variables are not padded;
a missing aggregate input stays an error.

Fixes benchmark-db bug #5 (BSBM BI query 4: an inner grouped sub-SELECT groups by
`?country`, which is bound only by the enclosing join). Pre-existing on main; not
subquery-specific (reproduces at a single query level).
mwatts pushed a commit to mwatts/fluree-db that referenced this pull request Jun 12, 2026
Two pieces that together let a multi-query envelope actually execute
against a single resolved snapshot moment with bounded parallelism.

Snapshot resolution (fluree-db-api/src/query/multi_snapshot.rs):

resolve_envelope_snapshot walks the distinct ledger set and produces an
EnvelopeSnapshot { as_of, ledgers: HashMap<String, i64> }. AsOf::T pins
the single ledger directly (multi-ledger with integer asOf was already
rejected at validation). AsOf::Iso resolves per-ledger via
time_resolve::datetime_to_t against each ledger's current snapshot,
mirroring the rounding rule in load_graph_db_at so sub-millisecond ISO
precision doesn't push us off-by-one before the intended commit. Absent
asOf captures server-now once via Utc::now() and pins each ledger to its
current t; the wall-clock moment is echoed back so the response is
reproducible. Per-ledger work is sequential in v1 — the connection-level
ledger cache (LedgerManager::get_or_load) coalesces repeated loads, so
the bounded fan-out (max_distinct_ledgers) is acceptable.

apply_snapshot_to_jsonld rewrites a sub-query body in place: string from
becomes object {@id, t}; object from gets t inserted next to its @id/id;
array entries get the same treatment recursively. Bare ledger ids are
matched against the snapshot map with temporal suffix and named-graph
fragment stripped, but the original identifier is preserved on the
written-back @id so fragment-selected named graphs still resolve.

apply_snapshot_to_sparql uses the SPARQL parser's IRI spans to do
precise byte-level splicing of FROM <iri> / FROM NAMED <iri> clauses,
appending @t:N to each ledger present in the snapshot map. Replacements
are sorted and applied right-to-left so earlier spans aren't shifted by
later edits. Span-based rewrite means IRIs inside comments or string
literals are never touched. When parsing fails the original string is
returned so the downstream parser produces the real error. IRIs that
already carry a temporal suffix are skipped defensively — validation
would have rejected the envelope before reaching here, but we don't
double-pin.

Dispatcher (fluree-db-server/src/routes/multi_dispatch.rs):

dispatch_multi_query owns a JoinSet for sub-query tasks and a Semaphore
sized to DispatchConfig::max_concurrency. Each task acquires its permit
before computing its effective timeout: min(opts.timeoutMs, remaining
envelope budget) measured at permit-acquisition time, not envelope-entry
— a sub-query that waited 30s on a 60s envelope gets <=30s regardless of
its own opts.timeoutMs. The envelope deadline is a select! arm against
sleep_until; when it fires JoinSet::abort_all() cancels in-flight tasks,
remaining slots are drained, and any task that didn't complete is
marked AliasOutcomeKind::Timeout with the envelope budget. Sub-query
errors land in ::Error with a coarse classification — task fluree#5/fluree#6 will
refine the code surface when assembling the HTTP response.

Per-task execution does the merge work: @context and opts merge via the
multi.rs helpers with sub-query winning; the merged context is injected
into JSON-LD bodies (or used to derive SPARQL PREFIX/BASE directives for
SPARQL sub-queries). After merge, apply_snapshot_to_jsonld /
apply_snapshot_to_sparql pin the per-ledger t and the result is passed
to run_jsonld_subquery / run_sparql_subquery — the connection-scoped
helpers from the earlier extraction. TrackingOptions is built once per
sub-query from the merged opts (track_time/fuel/policy + max_fuel) and
threaded through to the SPARQL builder; the JSON-LD path picks up
tracking from has_tracking_opts on the merged body.

Each sub-query gets its own sub_query span (alias, language,
effective_timeout_ms, result_status) as a child of whatever envelope
span the caller establishes. Span tagging happens both at task-start
(effective_timeout_ms once known) and at task-end (result_status).

Visibility: fluree-db-api's  is promoted to pub so the
server can reach query::multi and query::multi_snapshot; siblings
inside that module that were already pub mod stay accessible. The
server's mod query is promoted to pub(crate) so the new multi_dispatch
sibling can reuse the existing run_jsonld_subquery / SubqueryOutput
without duplicating their bodies.

12 snapshot unit tests cover JSON-LD application (string/object/array
from, named-graph fragment preservation, ledger not in map) and SPARQL
application (FROM and FROM NAMED rewrite, IRI inside string literal
untouched, unparseable returns original, ledger not in map, existing
suffix not overwritten). 6 dispatcher unit tests cover DispatchConfig
clamping to server bounds and the per-sub-query timeout fallback
ordering. Existing 47 server integration tests and 506 api lib tests
pass.
aaj3f added a commit that referenced this pull request Jul 2, 2026
Adversarial review follow-ups on the subject-key heuristic:

- (#4 parity) The identifier_field_ids branch now emits SubjectKeyUnverified for
  each key column - uniqueness is unverifiable metadata-only (NDV deferred) even
  for a declared identifier, matching the override and <STEM>_KEY/_ID branches.
  The non-null gate already handles composite keys (rejects if ANY member is
  nullable). Existing tests that asserted no diagnostic for a clean identifier
  hint are updated.

- (#5 emitter fixture) Adds an emit-level test that a partially-covered stat
  (null_fraction == None, unknown) on a non-`required` identifier is NOT treated
  as a safe subject key (is_non_null is false), while full coverage proving
  null_fraction == 0 makes the same column safe - composing the partial-coverage
  fix with the non-null gate. The shared fixtures only set null_fraction from
  `required`, so this builds the column explicitly.

Refs fluree/solo#724
aaj3f added a commit that referenced this pull request Jul 6, 2026
Address the review's findings — all verified real; all make the harness's
green trustworthy rather than changing what it covers:

- (review #1) the 'no unexpected named graph' guard was dead code: the
  engine binds GRAPH ?g as a plain literal, so the Iri-only filter in
  list_named_graphs always produced []. Accept literal and IRI bindings
  (excluding the alias-named default graph) so the guard survives the
  eventual engine fix. Verified live: an update leaving a stray graph with
  no expected graphs now fails with 'Unexpected non-empty named graph'.
- (review #2) bail when a manifest yields zero tests — a submodule
  restructure or manifest-parser regression must not report green.
- (review #3) a registered test that dies by timeout/subprocess crash now
  hard-fails: the register excuses a known wrong answer, not an infra
  death masking a new hang or panic.
- (review #4) UpdateEvaluationTest now rejects an mf:result blank node
  exposing none of ut:data/ut:graphData/ut:result, instead of degrading to
  a trivially satisfiable 'expected empty store'.
- (review #5) register entries matching no discovered test now fail the
  suite, so dead entries (typos, upstream renames) cannot accumulate.
- notes: tightened the TSV bare-integer heuristic to a single optional
  leading sign; documented the line-based directive-hoisting assumption;
  fixed the stale 'CI runs make ci' Makefile comment.

Full suite remains green: 36 suites, ~1420 tests, 0 failed, 0 ignored.
aaj3f added a commit that referenced this pull request Jul 12, 2026
Four additions, one per open review item:

- Multi-op atomicity on mid-request failure (review blocking #4): op 1
  INSERT DATA stages cleanly, op 2 fails at staging while evaluating
  over op 1's data (BIND(STRDT(?m, <bad:datatype>)) — proving op 2 ran
  against the sequential state); the request errors as a unit, 't' is
  unchanged, and op 1 leaves no trace.
- UPDATE BASE resolution at the seam (review blocking #2): the same
  BASE-carrying document stores and finds identical absolute IRIs on
  the update and query surfaces, GRAPH names included.
- DELETE WHERE label independence at the seam (review blocking #5):
  label reuse across DELETE WHERE ops executes, each op deleting its
  own matches.
- Indexed EXISTS-with-GRAPH-?g (review blocking #3): the
  GraphVarCorrelated strategy driven end-to-end on a binary-indexed
  ledger — including the string-literal back-compat — which no
  memory-backed suite (W3C harness included) ever exercised.

On the #1443 EncodedSid/EncodedLit extraction arms specifically, the
audit went one step past the review's finding: no current plan shape
delivers a still-encoded binding to either extraction site (probed:
plain join, OPTIONAL, FILTER EXISTS, UNION, string variant — upstream
operators materialize batches first). The arms stay as defense-in-depth
with their comment rewritten to say exactly that (this also retires the
stale 'never on a scan hot path' wording the review flagged).
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