Repository navigation
fix(query): key literals alike whichever join lane produced them - #1939
Conversation
The batched probe lanes keep xsd:date, xsd:time and xsd:dateTime objects encoded, while a scan over novelty decodes them, and so does the nested-loop join's batched lane on an indexed ledger. The equality surfaces turn decoded strings and numerics into their encoded form, but not these three, so a value reached both ways became two keys: COUNT(DISTINCT) over a UNION doubled it, GROUP BY split it into two groups, and MINUS kept the rows it should have removed. encoded_equivalent now encodes the three the same way late_materialized_object_binding does. Their keys are the canonical value, so nothing distinct is merged.
…ther lane The nested-loop join's batched lane built object bindings with its own copy of the decode rules, and #1729's fix never reached it. It still gave every string-dictionary datatype xsd:string's id, so literals reached through a join such as `ex:hub ex:ref ?s . ?s ex:p ?o` lost their datatype again: DATATYPE() reported xsd:string for xsd:anyURI, xsd:token and custom-typed values, and COUNT(DISTINCT) merged them into one. The lane now uses the builder that the property-join and OPTIONAL probes already share. It keeps the reserved string datatypes encoded, materializes the rest with their own datatype, and keeps dates, times and dateTimes encoded like the scan does.
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
@bplatz bad bug / great fix. Double-checked everything and both the problem and the solution hold up and are designed well. The only gap (not in design but coverage completion) seems to be on xsd:float on o_type. Full details below. It's not a regression but it's same class of bug, same files/function for the fix.
This is solid work to resolve a "wrong rows; no error" scenario. Deleting the nested-loop join's private copy of the object-binding rules instead of porting #1729's fix into it is the move that actually retires the class; a second copy of "which datatypes may borrow a reserved dict id" was always going to drift again. And the served_by lane assertions in it_literal_identity are the detail that makes me trust the tests: they pin that the lane you mean to exercise really served the query, in both the indexed and the with-novelty phase, so a future plan change can't quietly turn them into a test of a lane you already fixed.
I checked rather than took the body's word. Reverting only join.rs and object_binding.rs to 09f15cb33 turns both new tests red with exactly the values you describe — COUNT(DISTINCT ?d) = 6 instead of 3, and DATATYPE(?o) = xsd:string for all four of xsd:string/custom/xsd:anyURI/xsd:token — while the five pre-existing tests in the binary stay green. Disabling only the xsd:date arm of encoded_equivalent gives 4 instead of 3, so that arm is independently load-bearing. And the temporal keys really are the indexer's: ObjKey::encode_date(d.days_since_epoch()) here is the same call run_index/resolve/resolver.rs:920-929 makes, with offsets already normalised at parse per temporal.rs's module docs, so nothing distinct gets merged.
One thing I'd like folded in while you're in the function: xsd:float has the identical split-key bug and isn't covered. inline_numeric_encoding keeps it encoded as (NUM_F64, DatatypeDictId::FLOAT), but encoded_equivalent's Double arm guards on xsd:double, so a decoded float falls through. Claude ran your temporal test shape retyped to xsd:float and COUNT(DISTINCT ?d) comes back 6 instead of 3 at this PR's HEAD. Pre-existing, not yours, and non-blocking — but it's four lines next to the ones you just wrote, and by the same reading @fulltext / JSON / vector look like they may sit in the same gap (I didn't construct those).
On performance, since this is the join path: unchanged for xsd:string/langString/JSON/vector/NUM_BIG/the reserved numerics; faster for date/time/dateTime through the NLJ batched lane, which now stays encoded instead of decoding and re-interning per row; and slower for non-reserved string datatypes through that lane, which now cost a dictionary read plus a String per row. That last one is the correctness fix itself and it's the same trade the property-join and OPTIONAL probes already pay, so I'd call the whole thing net-neutral-to-positive with one justified regression rather than an unaccounted one.
Adherence to repo commitments:
- Patterns/abstractions: ✔ Routes the last divergent lane through the shared
build_probe_object_bindingand deletes the duplicate; extends the singleencoded_equivalentnormalisation point rather than teaching each equality surface about temporal types. - Performance (speed first, memory second): ✔ Join path touched and accounted for per-case above; the one slower path is the fix, matching the trade the other probe lanes already make. No new locks, allocations or
.collect()intermediates that weren't required. - Testing: ✔ Two integration tests in a real
[[test]]target (it_literal_identity,required-features = ["native"]), both phases, both surfaces, lane-asserted; verified running by name, red on the merge base, and red under an isolated single-arm mutation. - Conventions: ✔ Self-describing subjects, thorough multi-line bodies that explain the required commit order, clippy/fmt/W3C green on
ff3db3f73(run36146167803,event=pull_request).
Verified locally at branch HEAD: cargo nextest run -p fluree-db-api --features native --test it_literal_identity → 7 passed; base revert → the 2 new tests fail as described; xsd:date-only mutation → 4 vs 3; xsd:float probe → 6 vs 3 at HEAD. Worktree restored clean.
Approving so you can merge when ready — the xsd:float arm is the only thing I'd try to get in before this lands, and it shouldn't hold anything up.
Not anchorable inline: one more bit of praise that belongs to the commit structure rather than to a line — "This fix has to land first: routing the nested-loop join lane through the shared builder without it would move the date mismatch from indexed ledgers to ledgers with novelty" is exactly the sentence a reviewer needs, and the commits are in that order. Worth noting the consequence out loud too: it means neither commit is individually revertable, which is the kind of thing that matters six months from now.
| } | ||
| // The temporal types `embedded_temporal_encoding` keeps | ||
| // encoded. Their keys are the canonical value itself. | ||
| (FlakeValue::Date(d), DatatypeConstraint::Explicit(dt)) |
There was a problem hiding this comment.
Optional — the same bug is one arm over: xsd:float isn't covered.
inline_numeric_encoding maps OType::XSD_FLOAT → (NUM_F64, DatatypeDictId::FLOAT), so the scan keeps a float encoded — but encoded_equivalent's Double arm just above guards on *dt == Sid::xsd_double(), so a decoded xsd:float falls through to _ => return None. Same split key, same surfaces as the temporal types you just fixed.
Claude ran your own temporal_literals_keep_one_key_across_join_lanes shape retyped to xsd:float — three distinct values stored twice each, novelty layered over the index — and COUNT(DISTINCT ?d) comes back 6 instead of 3 at this PR's HEAD. So it's live, just pre-existing rather than introduced here.
It looks like four lines next to the ones you just added:
(FlakeValue::Double(v), DatatypeConstraint::Explicit(dt))
if is_xsd(dt, xsd_names::FLOAT) =>
{
let key = fluree_db_core::value_id::ObjKey::encode_f64(*v).ok()?;
(
ObjKind::NUM_F64.as_u8(),
key.as_u64(),
DatatypeDictId::FLOAT.as_u16(),
0,
)
}Worth double-checking that a decoded xsd:float really arrives as FlakeValue::Double with an explicit xsd:float constraint — that's the assumption behind the snippet, and I inferred it from inline_numeric_encoding rather than reading the decode path. By the same reading @fulltext, JsonArena and VectorArena are also encoded by the scan with no encoded_equivalent arm, though I didn't construct those, so I'd treat them as worth a look rather than as established.
Minor and non-blocking — but you're already in the function and the mechanism is identical, so I'd rather see it folded in now than lost in the backlog.
There was a problem hiding this comment.
Folded in, in b59c215.
Your assumption about the decode shape holds. A stored xsd:float decodes through DecodeKind::F64 to FlakeValue::Double, and resolve_datatype_sid_for_value gives it xsd:float. So it now shares the Double arm and picks DatatypeDictId::FLOAT by datatype.
@fulltext and JSON were live too. Both go through the string dictionary (JSON despite its JsonArena decode kind), and both gave the same 6-instead-of-3 on your test shape. The same commit adds an arm for each. @fulltext shares the string arm; JSON is keyed by its serialized text.
scan_encoded_literals_keep_one_key_across_join_lanes runs the temporal test body (now a shared helper) for all three, over both lanes, both phases and the JSON-LD case. Float was red before the fix, and disabling the @fulltext or JSON arm alone turns it red again.
Vector is a different mechanism. Its arena is per (graph, predicate), like NUM_BIG, so a handle isn't a canonical key and there's nothing to encode to. I reproduced it, and it's worse than a split key: EncodedLit equality ignores p_id for VECTOR_ID, so vectors on different predicates with the same handle compare equal. SELECT DISTINCT drops one, and MINUS over-removes. Filed as #1957 with options, including just documenting a narrower rule, and left out of this PR.
| } | ||
| } | ||
|
|
||
| fn is_xsd(dt: &Sid, name: &str) -> bool { |
There was a problem hiding this comment.
Nit — take it or leave it: now that is_xsd exists, the older arms could use it too.
The three arms just above still spell the same test three different ways: *dt == Sid::xsd_string(), an inline dt.namespace_code == XSD && dt.name.as_ref() == "long", and *dt == Sid::xsd_double().
Not worth a commit on its own. But if you touch the float arm anyway, folding those onto is_xsd would make the next addition to this match obviously correct rather than requiring a reader to check three spellings.
There was a problem hiding this comment.
Done in the same commit. Every arm now checks its datatype through is_xsd, and the Long/Double arms pick their dict id by datatype rather than using a guard, so the float addition is just another branch.
| let o_i = batch.o_i.get_or(row, u32::MAX); | ||
| let t = batch.t.get_or(row, 0) as i64; | ||
| self.build_batched_object_binding( | ||
| build_probe_object_binding( |
There was a problem hiding this comment.
Praise — deleting the lane's private copy rather than porting #1729's fix into it is the move that actually retires the class.
A second copy of "which datatypes may borrow a reserved dict id" was always going to drift again the next time that rule moved — which is precisely what happened here. 163 lines of duplicated decode rules gone, and the nested-loop join now shares the builder the property-join and OPTIONAL probes already use.
This is the reason I believe this one is fixed rather than patched.
| const BATCHED_OPTIONAL_LANE: &str = "optional batched probe complete"; | ||
|
|
||
| /// Runs `run`, asserting the lane that logs `lane` served it. | ||
| async fn served_by<F: std::future::Future<Output = Value>>( |
There was a problem hiding this comment.
Praise — the lane assertion is what keeps a test like this honest.
served_by checking the debug event to confirm that the lane you mean to exercise actually served the query, in both an indexed and a mixed phase, is the detail that stops a future plan change from quietly turning these into tests of a lane you already fixed. Without it, the strongest test here could pass for the wrong reason and nobody would know.
I'd like to see that pattern spread to the other lane-specific suites.
…oduced them The scan keeps these three encoded, like the temporal types, but `encoded_equivalent` had no arm to encode their decoded form. A value reached through a batched probe lane and through a scan over novelty was two DISTINCT keys, two groups, and survived its own MINUS. xsd:float decodes to a Double, so it joins the xsd:double arm under its own reserved id. @fulltext and JSON share the string dictionary, keyed by the lexical form and the serialized text respectively. The older arms now spell their datatype checks through `is_xsd`.
2 participants
Two bugs on
mainwhere the same literal gets a different binding depending on which join lane produced it. Both show up in query results.String datatypes through a nested-loop join
The nested-loop join's batched lane had its own copy of the object-binding rules, and #1729's fix never reached it. It still gives every string-dictionary datatype
xsd:string's id. Withex:s1–ex:s4holding"abc"asxsd:string, a custom datatype,xsd:anyURIandxsd:token, on an indexed ledger:The lane now uses
build_probe_object_binding, which the property-join and OPTIONAL probes already share, and its copy is deleted.Temporal values keyed two ways
The shared builder keeps
xsd:date,xsd:timeandxsd:dateTimeencoded, but a scan decodes them when there is novelty, and the nested-loop join lane decoded them always.DISTINCT,GROUP BYandMINUSconvert decoded strings and numbers to their encoded form before comparing, but they skipped these three. So a value reached through two lanes counted as two different values:GROUP BY ?dsplit each value into two groups, andMINUSkept rows it should have removed. On an indexed ledger with no novelty, the nested-loop join lane caused the same thing.encoded_equivalentnow encodes the three types the same waylate_materialized_object_bindingdoes. Their keys are the canonical values (offsets are normalized at parse), so no two distinct values end up merged.This fix has to land first: routing the nested-loop join lane through the shared builder without it would move the date mismatch from indexed ledgers to ledgers with novelty.
The rest of what the scan keeps encoded
xsd:float,@fulltextand JSON had the same gap: the scan keeps them encoded, butencoded_equivalenthad no arm for their decoded form, so each showed the same doubledCOUNT(DISTINCT)across aUNIONof lanes. A decodedxsd:floatis a double, so it now shares thexsd:doublearm under its own reserved id.@fulltextand JSON are keyed through the string dictionary, by lexical form and by serialized text. The older arms now check their datatype throughis_xsd.Tests
Both are in
it_literal_identity. Each runs with and without novelty over the index and asserts, via its debug event, that the intended lane actually ran:string_dict_datatypes_keep_their_identity_through_a_subject_joinchecksDATATYPEandCOUNT(DISTINCT)in SPARQL and JSON-LD. It fails onmain. It also fails when only the novelty call site keeps the old builder; that case is theex:s5row.temporal_literals_keep_one_key_across_join_laneschecksCOUNT(DISTINCT),GROUP BYandMINUSacross aUNIONof each lane with a plain scan, plus a JSON-LD case. It fails without the normalization.scan_encoded_literals_keep_one_key_across_join_lanesruns the same checks forxsd:float,@fulltextand JSON. It fails without any one of the three arms.Follow-up: #1957. Vector literals compare by arena handle, a separate mechanism this PR does not touch.