feat(r2rml): bound subjects, decimal/double/date & ref object constants, star fusion, subject-key pushdown - #1413
Conversation
A triple pattern with a constant subject (`<store/5> ex:name ?n`) previously returned None from convert_triple_to_r2rml and was left unconverted, which the graph rewriter treats as a hard error. Make R2rmlPattern.subject_var optional and add subject_constant: the operator materializes each row's subject from the template and keeps only rows whose subject IRI equals the constant, binding just the object var(s). Enforced as the pattern's semantics (independent of scan pushdown), mirroring how constant IRI objects work. Grouping (star/class fusion) is restricted to variable-subject patterns via star_eligible_subject/class_only_subject returning the subject VarId.
Previously a triple with a decimal/double/date literal object (`?s ex:price 9.99`, `?s ex:when "2024-01-15"^^xsd:date`) returned None from convert and was left unconverted, which the graph rewriter treats as a hard error. Extend const_object to cover them: - Date → Scalar(ScanValue::Date), matched by parsing the materialized ISO-8601 lexical back to days-since-epoch; also emits a scan filter for pruning. - Decimal / big-integer → new ObjectConstant::Decimal, scale-insensitive numeric match (`9.99` matches a `9.990` column). Operator-enforced only. - Double → new ObjectConstant::Double, exact f64 value match. Operator-enforced only. The decimal/double paths add no scan pushdown (which would need decimal-aware Iceberg predicates), so correctness rests entirely on the operator, consistent with how IRI constant objects already work.
A ref object expressed as a typed value (Term::Value(FlakeValue::Ref)) fell into the literal arm, returned None, and was rejected (hard error in the graph rewriter). Decode it to an IRI so it takes the same operator-enforced path as a Term::Sid / Term::Iri ref object.
A constant-object triple (`?s ex:storeId "STORE-2"`) that shares a subject with a var-object star (`?s ex:name ?n`) previously ran as its own scan joined back on the subject. Fold it into the star as an equality existence constraint (star_constraints): the operator materializes the predicate's objects for each row and keeps the row only when one equals the constant, producing no extra variable and eliminating the self-join. star_member_subject now admits both var-object and constant-object members; the star emitter partitions them, fusing constants onto a var-object base. Groups with no var-object base keep their constant-object members standalone. Operator-enforced (no scan pushdown for the fused constraint yet), so correctness holds with pruning off. Guard proves dw.store is scanned exactly once.
A bound subject (`<store/5> ex:name ?n`) previously always scanned the whole table and filtered in-engine. Add reverse_subject_template — the exact inverse of expand_template + iri_escape — which recovers each key column's raw value from the constant IRI, and push it to the Iceberg scan as an equality so the reader can prune to the matching rows. The operator still enforces the subject equality, so this only affects which rows the scan returns, never the result. Reversal is unambiguous only for a single trailing placeholder or multi-placeholder templates whose inter-placeholder separators are always percent-encoded (`/`); it bails to a full scan otherwise. The physical type is resolved against the Iceberg schema in build_iceberg_filter via a new ScanValue::TemplateKey: integer keys on int/long/decimal columns push as an integer literal (the Arrow reader casts to a Decimal column — the already-validated integer-vs-decimal path), string keys push as a string, and unsupported types fall back to a full scan. Gated OFF by default behind FLUREE_R2RML_SUBJECT_KEY_PUSHDOWN pending live validation. Tests: template round-trip (incl. slash/space/unicode/%), physical-type coercion, and end-to-end filter emission. Docs: R2RML subject-key pushdown section (percent-encoding contract + query-authoring rule) and the config flag.
…ing switch The dedicated FLUREE_R2RML_SUBJECT_KEY_PUSHDOWN flag was redundant: subject-key filters flow through the same reader-level machinery (surviving_row_groups + the Arrow post-decode filter) already governed by FLUREE_ICEBERG_PREDICATE_PUSHDOWN, and the operator remains authority regardless. Emit the reversed key filter unconditionally, exactly like the object-constant filters, so bound-subject pushdown is on by default and kill-switchable via the existing flag — no new product flag.
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
Looks great and like the "operator stays authority; pushdown is an optimization" -- just relaying some correctness or perf gaps below:
There was a problem hiding this comment.
pattern_predicates() (the source for both the star projection and RefObjectMap parent-lookup construction) includes predicate_filter and star_bindings but not star_constraints:
fn pattern_predicates(&self) -> Vec<&str> {
let mut preds = Vec::new();
if let Some(p) = self.pattern.predicate_filter.as_deref() { preds.push(p); }
for (pred, _) in &self.pattern.star_bindings { preds.push(pred.as_str()); }
preds // star_constraints predicates are never included
}For the headline case ?s ex:name ?n ; ex:storeId "X", the base has a single var-member (ex:name), so star_bindings is empty and star_constraints = [(ex:storeId, "X")]. The projection then takes the star_bindings.is_empty() branch (operator.rs:548) -> columns_for_predicate(Some("ex:name")), which projects the subject columns + store_name but not store_id. The real provider prunes to exactly that projection (fluree-db-api/src/graph_source/r2rml.rs:~1010, .with_projection(projected_field_ids)), so store_id is absent from the batch. In materialize_batch the constraint loop (operator.rs:1335-1358) calls materialize_pom_object for ex:storeId, whose column read returns None → matched=false → row_ok=false → every subject row is dropped.
Failure scenario: mapping dw.store(store_key, store_name, store_id), ex:name→store_name, ex:storeId→store_id; data STORE-2 = ("Bravo","STORE-2"); query ?s ex:name ?n ; ex:storeId "STORE-2". Expected 1 row; a real scan returns 0. The unfused standalone form (?s ex:storeId "STORE-2" alone) works, so fusion is a strict correctness regression versus not fusing. Same root cause hits the RefObjectMap variant (parent lookups are built only for star_preds, operator.rs:~650).
Why tests miss it: guard_constant_object_fuses_into_star_single_scan asserts rows==1, but CountingProvider (and the other star mocks) ignore the projection and return the full pre-built batch — so store_id is present in the mock. Only the real reader prunes columns.
Fix: include star_constraints predicates in pattern_predicates() (fixes both the projection and filtered_poms parent-lookup construction), and add a projection-honoring mock (or a real-reader integration test) that would have caught it.
| // Seed a fresh output row with the subject binding, or an empty row when | ||
| // the subject is a constant (which binds no variable). | ||
| let seed_row = || -> Vec<(VarId, Binding)> { | ||
| match pattern.subject_var { | ||
| Some(sv) => vec![(sv, subject_binding.clone())], | ||
| None => Vec::new(), | ||
| } | ||
| }; |
There was a problem hiding this comment.
seed_row adds an alloc+realloc (and a clone) per emitted row in the two commonest scan paths
let seed_row = || -> Vec<(VarId, Binding)> {
match pattern.subject_var {
Some(sv) => vec![(sv, subject_binding.clone())], // capacity 1
None => Vec::new(),
}
};
// single-object path:
let mut row = seed_row(); // alloc cap-1
row.push((obj_var, object_binding)); // full → realloc to cap-2 (+ memcpy + free)The old single-object code was produced.push(vec![(sv, subject_binding.clone()), (obj_var, ob)]) — one allocation at capacity exactly 2, no realloc. The new code allocs cap-1 then reallocs to cap-2 per row → ~2× allocator traffic per output row in the hottest R2RML path (a scan can emit millions of rows). Separately, the subject-only path (operator.rs:~1414) old code moved subject_binding into the row; because seed_row borrows it, it now clone()s every row (an extra Arc<str> refcount bump / Sid copy). Both regressions fire even when none of the new features is used.
Fix: don't route the common paths through the closure.
Single-object: let mut row = Vec::with_capacity(2); if let Some(sv) = pattern.subject_var { row.push((sv, subject_binding.clone())); } row.push((obj_var, object_binding));
Subject-only: produced.push(match pattern.subject_var { Some(sv) => vec![(sv, subject_binding)], None => Vec::new() }); (move, no clone). Keep the closure only for the star branches that need multiple copies.
| }, | ||
| Some("string") => LiteralValue::String(s.clone()), | ||
| _ => continue, | ||
| }, |
There was a problem hiding this comment.
The Date arm emits a Date literal unconditionally, unlike Int and TemplateKey which gate on field.type_string():
ScanValue::Date(d) => LiteralValue::Date(*d), // no field.type_string() check
ScanValue::Int(n) => match field.type_string() { Some("int") => ..., _ => continue }, // guarded
ScanValue::TemplateKey(s) => match field.type_string() { ... }, // guardedbuild_scan_filters pushes the Scalar(Date) constant against the object column with no physical-type check, and the Arrow reader applies it as an exact row filter (eval_comparison casts Date32 → Utf8 = "2024-01-15"). But the operator enforces with lenient Date::parse (operator.rs:~1242), which accepts "2024-01-15Z" / "2024-01-15+05:00". So for an xsd:date object mapped onto a physically string column holding "2024-01-15Z", the operator keeps the row but the pushed row filter drops it ("2024-01-15Z" != "2024-01-15") → missing rows, violating the "pushdown never removes an operator-kept row" invariant the code states at operator.rs:~1204. Default-on.
Fix: gate the Date arm on field.type_string() == Some("date"), matching the other arms. (Related to the templated/transformed-object-map pushdown gap noted in the #1411 review; this is a distinct, cleanly-fixable instance.)
| // The separator must begin with a hard (always-escaped) char so the | ||
| // value's right boundary is unambiguous. | ||
| if !sep.chars().next().is_some_and(is_always_escaped) { | ||
| return None; | ||
| } | ||
| let idx = rest.find(sep)?; |
There was a problem hiding this comment.
a %-led separator passes the is_always_escaped guard but collides with %XX inside encoded values → wrong recovered key → silent missing results
The separator-boundary guard only checks the separator's first char is always-escaped:
if !sep.chars().next().is_some_and(is_always_escaped) { return None; }
let idx = rest.find(sep)?;The reasoning ("a hard delimiter can never appear literally inside an encoded value") has one hole: % is always-escaped (a literal % in a source value encodes to %25), so is_always_escaped('%') is true — but % is also the escape-introducer that begins every %XX byte inside an encoded value. So a separator starting with % passes the guard yet rest.find(sep) can match a false
boundary inside a value.
Failure scenario: template http://ex/person/{first}%20{last} (a %20/space composite-key separator), row first="Mary Ann", last="Smith" → generated/queried IRI http://ex/person/Mary%20Ann%20Smith. Reversal finds %20 at index 4 (inside first's encoded space), recovering first="Mary", last="Ann Smith" — wrong keys. The pushdown then filters first Eq "Mary" AND last Eq "Ann Smith", pruning the actual row before the operator's subject check runs → the exact subject the user queried returns nothing. Default-on; no test uses a %XX separator.
Fix (precise): % is provably the only always-escaped char in iri_escape's output alphabet, so reject a %-led separator: if !sep.chars().next().is_some_and(|c| is_always_escaped(c) && c != '%') { return None; } — such templates then fall back to a full scan (correct, just unpruned).
(Note: is_always_escaped is otherwise the exact complement of iri_escape's literal set, and percent_decode / strip_prefix / the ambiguous-shape bails are all correct — verified char-by-char. This one lead-char case is the sole hole.)
| // Regular predicate pattern: ?s ex:name ?o | ||
| // Extract predicate IRI filter - handle both Ref::Sid (decode) and Ref::Iri (use directly) | ||
| let predicate_filter = match &tp.p { | ||
| Ref::Sid(sid) => snapshot.decode_sid(sid), | ||
| Ref::Iri(iri) => Some(iri.to_string()), | ||
| Ref::Var(_) => None, // Predicate is variable - no filter | ||
| }; |
There was a problem hiding this comment.
With a bound subject and a variable predicate, predicate_filter = None (Ref::Var(_) => None) and object_var = Some(?o). Pre-PR, bound subjects returned None and the triple was preserved for normal evaluation; now it converts to a bound-subject pattern with no predicate-variable field, so ?p is dropped. At materialize time predicate_filter=None matches every POM, binding ?o to every object of store/5 across all predicates with ?p left unbound. SELECT ?p ?o WHERE { <store/5> ?p ?o } returns objects with ?p = NULL instead of (predicate, object) pairs — a wrong-results regression for that shape (the var-subject ?s ?p ?o wildcard has the same latent limitation, but this PR newly routes bound-subject wildcards into it).
Fix: in convert_triple_to_r2rml, return None when predicate_filter is None and the object is a variable, leaving the triple for normal evaluation.
| // Decimal / big-integer object: numeric (scale-insensitive) match, so a | ||
| // query `9.99` matches a column materialized as `9.990`. | ||
| ObjectConstant::Decimal(d) => { | ||
| let RdfTerm::Literal { value: v, .. } = term else { | ||
| return false; | ||
| }; | ||
| v.parse::<bigdecimal::BigDecimal>().is_ok_and(|x| &x == d) | ||
| } | ||
| // Double object: exact f64 value match. |
There was a problem hiding this comment.
decimal object match parses a fresh BigDecimal per row across the whole scan
ObjectConstant::Decimal is not pushed as a scan filter (build_scan_filters pushes only Scalar), so a query ?s ex:price 9.99 reads every row and v.parse::<BigDecimal>() heap-allocates a digit vector per row. Correct, but avoidable: add a lexical fast-path (v == d.to_string() || v.parse…) or push a numeric scan filter so the reader prunes first. (The Double/f64 arm is fine — parse::<f64> doesn't allocate.)
| // A reversed subject-template key: coerce the raw string to the | ||
| // column's physical type. Integer keys on `int`/`long`/`decimal` | ||
| // columns push as an integer literal (the Arrow reader casts it to a | ||
| // Decimal column, and row-group stats conservatively skip decimals), | ||
| // string keys push as a string. Non-integer or unsupported physical | ||
| // types (float/date/timestamp/boolean, non-integer decimals) skip the | ||
| // pushdown — the operator still enforces the subject equality. | ||
| ScanValue::TemplateKey(s) => match field.type_string() { | ||
| Some("int") => match s.parse::<i32>() { | ||
| Ok(v) => LiteralValue::Int32(v), | ||
| Err(_) => continue, | ||
| }, | ||
| Some(t) if t == "long" || t.starts_with("decimal") => match s.parse::<i64>() { | ||
| Ok(v) => LiteralValue::Int64(v), | ||
| Err(_) => continue, | ||
| }, |
There was a problem hiding this comment.
scale>0 decimal key columns ARE pushed, contrary to the docstring
Some(t) if t == "long" || t.starts_with("decimal") matches decimal(10,2), but the doc comment and docs/graph-sources/r2rml.md say only integer-valued decimals are pushed. It's sound (an integer key only parses to i64 when the column materializes without a point, so the operator's subject-IRI equality keeps a superset of ∅; a real "5.00" key fails i64 parse and is skipped), so no missing rows — but the comment overstates what prunes. Correct the wording to avoid a future reader relying on it.
All three are default-on wrong-results in this PR's new query shapes: - Constant-object star fusion dropped every row on the real reader. pattern_predicates() omitted star_constraints, so a fused `?s ex:name ?n ; ex:storeId "X"` projected only the base predicate's columns; the column-pruning reader then lacked store_id, the constraint materialized as NULL, and every subject row was dropped. Include star_constraints in pattern_predicates() and route base+constraint patterns through the star projection / parent-lookup path (has_star_members). The star-fusion guard now asserts the projection includes the constraint column (the mock returns the full batch, so a row count alone never caught it). - Bound subject + variable predicate (`<store/5> ?p ?o`) converted to a pattern with no predicate_filter and no predicate-var binding, binding ?o across every predicate with ?p left NULL. Leave that shape unconverted for normal evaluation; the var-subject wildcard is unchanged. - Date constant-object scan filter was pushed without a physical-type gate. On a physically-string column the operator's lenient Date::parse keeps "2024-01-15Z"/offset forms that the exact row filter drops. Gate the Date arm on a physically-date column, matching the other arms. Adds regression tests for each.
…e in scan - Subject-key template reversal accepted a '%'-led separator (e.g. '%20'). '%' is always-escaped so it passed the hard-delimiter guard, but it also introduces every '%XX' byte inside an encoded value, so `find(sep)` could match a false boundary and recover wrong keys — pruning the very row the query asked for. '%' is the only always-escaped char in iri_escape's output, so reject '%'-led separators (fall back to a full scan). Gated behind the off-by-default subject-key pushdown. Regression test added. - The seed_row closure allocated a capacity-1 subject row then reallocated to capacity-2 on the object push in the single-object path, and cloned the subject binding in the subject-only path — both in the hottest scan path, firing even with none of the new features. Allocate at the final capacity for single-object and move (not clone) the subject binding for subject-only; the star cross-product keeps its clone (it needs copies).
The subject-key pushdown fires for a decimal column of any scale when the recovered key value parses as an integer (pushed as Int64, cast to the column's decimal type by the Arrow reader) — not only for integer-valued (scale-0) decimal columns as the docstring and docs implied. A non-integer key value, not a scale>0 column, is what skips the pushdown. Wording only; behavior is unchanged and the operator always re-enforces subject equality.
A decimal constant-object match (`?s ex:price 9.99`) parsed the materialized value into a fresh BigDecimal per row across the whole scan (a heap allocation each), since the decimal object is operator-enforced and not pushed as a scan filter. Precompute the constant's canonical string once per batch and short-circuit on an exact lexical match (the common same-scale case), falling back to the exact scale-insensitive BigDecimal compare only for scale variants. The numeric compare remains the authority.
Resolve rdf_term_eq_object_constant conflict in r2rml/operator.rs: main added a numeric_column parameter (integer-literal vs decimal-column matching) while this branch added Decimal/Double object constants plus a decimal_canonical fast-path. The two are complementary, so the merged rdf_term_eq_object_constant_cached carries both; a 3-arg cfg(test) wrapper keeps main's existing tests unchanged. Scan call sites pass both the per-column numeric flag and the precomputed canonical string.
2 participants
Summary
Expands R2RML/Iceberg graph-source query support along two axes: eliminating query shapes that used to hard-error (correctness-first) and adding an opt-in bound-subject pushdown. Stacked on
feature/iceberg-arrow-reader.Previously, any triple pattern the R2RML rewriter couldn't convert was left unconverted, which the graph rewriter treats as a hard error. Several common shapes hit that path. Each fix below is operator-enforced (the operator is authority), so results are correct with scan pushdown off and are verifiable with mock providers.
Changes
<store/5> ex:name ?n):R2rmlPattern.subject_varis nowOptionplus asubject_constant; the operator materializes each row's subject and keeps only matches, binding just the object var(s).?s ex:price 9.99,?s ex:when "2024-01-15"^^xsd:date): decimals match numerically (scale-insensitive,9.99==9.990), doubles by f64 value, dates by parsing the materialized ISO-8601 lexical back to days-since-epoch. Decimal/double are operator-only; date also emits a scan filter.FlakeValue::Refare now decoded to an IRI constant instead of being rejected.?s ex:name ?n ; ex:storeId "X") folds into the star as an existence constraint, eliminating a separate scan + self-join.reverse_subject_template— the exact inverse of the R2RML template expansion + percent-encoding — recovers the key column value from a constant subject IRI and pushes it to the Iceberg scan as an equality predicate for pruning. Gated behindFLUREE_R2RML_SUBJECT_KEY_PUSHDOWNpending live validation on production backends. The operator still enforces the subject equality, so enabling it only changes which rows the scan returns, never the result.Correctness notes
west/5→.../store/west%2F5). A mis-encoded IRI matches no subject identically whether pushdown is on or off, so it can never cause a divergence — only a query-authoring footgun. Documented indocs/graph-sources/r2rml.md.Tests
/, space, unicode, literal%), ambiguous-shape bails, non-matching IRIs.build_iceberg_filterTemplateKey → int/long/decimal/string, skip otherwise).store_keyfilter and the result stays correct via operator authority.Docs
docs/graph-sources/r2rml.md: bound-subject key pushdown section (encoding contract + query-authoring rule).docs/operations/configuration.md:FLUREE_R2RML_SUBJECT_KEY_PUSHDOWNflag.