Repository navigation
fix(query): indexed overflow integers keep xsd:integer through DATATYPE, DISTINCT, GRAPH and ORDER BY - #2012
Conversation
…PE, DISTINCT, GRAPH and ORDER BY An integer beyond i64 shares the NUM_BIG arena with big decimals, and the scan emits it as an EncodedLit whose dt_id is DECIMAL whatever the value. Only the decoded value can tell xsd:integer from xsd:decimal. #1329 fixed two decode sites (the materializer and the API formatter); the rest still read the datatype straight from dt_id: - group_aggregate::materialize_encoded built Lit{BigInt, xsd:decimal}, so DISTINCT, GROUP BY and MINUS keyed the scanned copy apart from a BIND or VALUES copy of the same integer, and a value leaving a GRAPH scope rendered as xsd:decimal. - sort.rs's materialization did the same, so ORDER BY output rendered as xsd:decimal. - DATATYPE() mapped dt_id without decoding, so it reported xsd:decimal and FILTER(DATATYPE(?o) = xsd:integer) dropped the row. BinaryIndexStore::resolve_dt_id_sid_for_value is the dt_id-keyed twin of resolve_datatype_sid_for_value: the decoded value's variant wins over dt_id. Every site that turns a decoded EncodedLit into a typed term now resolves its datatype there, including the transaction WHERE materializer. DATATYPE() decodes only when the binding is NUM_BIG. The unused DictOverlay::decode_dt_sid, a dt_id-only resolver, is removed.
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
✅ Approving to unblock you — with 1 thing that needs to be addressed before this merges.
Must address: the transaction WHERE materializer at fluree-db-transact/src/stage.rs:3193 is reachable, and before this PR it committed indexed overflow integers as xsd:decimal. With only that file reverted, INSERT { ex:acct ex:copy ?o } WHERE { ex:acct ex:serial ?o } on your fixture writes the copy as "1234…890"^^xsd:decimal. Your fix is right, but it's the only fix in the PR that changes what gets committed, and the only one with no test. The body also says no test reaches it, which isn't the case. The inline comment has a test I ran (red on the base stage.rs, green at the head).
Beyond that, this looks really good to me. Folding the two #1329 sites and the four new ones onto resolve_dt_id_sid_for_value, and deleting the dt_id-only resolver, is the right shape: the next decode site has one helper to copy. I went looking for a site you missed and couldn't find one. I grepped every dt_sids() read and EncodedLit match across fluree-db-query, fluree-db-api and fluree-db-transact (the remaining reads are the temporal checks, the string-dict join probe and the non-NUM_BIG DATATYPE arm, none of which see a NUM_BIG binding). Then I ran about 30 probe queries against an indexed overflow integer: =, sameTerm, IN, bound-object and VALUES joins, OPTIONAL, MIN/MAX/SAMPLE, COUNT(DISTINCT), an alternative property path, subqueries, GRAPH ?g, COALESCE/IF/BIND copies, CONSTRUCT, typed JSON, TSV, and FROM over two graphs of the ledger. Every one reports xsd:integer at the head. Your COUNT(DISTINCT) fast-path note holds too: NumBigDistinctKey keys Int apart from Dec (fast_count.rs:1342), and so does the general pipeline, before and after.
🟠 Should address (pre-existing, not from this PR): while probing that path I found that the caller next door commits the wrong value for a big number read under USING. materialize_encoded_bindings_for_txn decodes every encoded binding through graph 0 (fluree-db-transact/src/stage.rs:3103, BinaryGraphView::new(Arc::clone(&store), 0)), but arena handles are numbered from 0 per graph and predicate. I indexed ex:a ex:v 123456789012345678901234567890 in the default graph and ex:b ex:v 1234567890123456789012345678.5 in <g>, then ran INSERT { ex:z ex:copy ?o ; ex:from ?s } USING <g> WHERE { ?s ex:v ?o }. It binds ?s = ex:b but commits ?o = 123456789012345678901234567890, which is the default graph's value. The same update with GRAPH <g> { … } inside the WHERE commits the right value, because the GRAPH exit decodes through <g>. For a single USING/WITH graph the fix looks small: pass the WHERE's base graph id (the [Some(g_id)] arm of the base_db match at stage.rs:2781) into this function instead of 0. A merged default graph from several USING clauses is harder, since a batch there has no single graph to decode through. This is a different bug from the one the PR set out to fix, but it's the same function and it silently corrupts committed data, so I'd lean toward folding the single-graph fix in here. Happy to talk it through if you'd rather split it out.
One question, more for the record than for this PR. The reason any of this needs a decode is that OTypeRegistry::resolve sends every ObjKind::NUM_BIG to NUM_BIG_OVERFLOW whatever its dt (fluree-db-core/src/o_type_registry.rs:86), so the index also drops integer subtypes of overflow values. "18446744073709551615"^^xsd:unsignedLong and a 30-digit xsd:nonNegativeInteger report their own datatypes from novelty and xsd:integer once indexed (xsd:decimal from DATATYPE before this PR). This PR moves that closer to right, and fixing it fully needs the arena or the o_type to carry the datatype, which is an index-format decision, so I don't think it belongs here. Do we have that tracked anywhere? It's the same novelty-vs-index split this PR closes for plain integers.
Adherence to repo commitments:
- Patterns/abstractions: ✔ One helper next to #1329's o_type-keyed twin, with every decode site routed through it; the
dt_id-only resolver is removed instead of left to drift. - Performance (speed first, memory second): ✔ Neutral.
DATATYPE()decodes only NUM_BIG bindings and adds oneu8compare for the rest;materialize_encodedandsort.rsadd one discriminant match per decoded literal, and theLazyLockSidclone costs the sameArcbump as thedt_sids()clone it replaces. Nothing new per row on the DISTINCT/GROUP BY path. - Deployment targets: ✔ n/a. Pure in-memory engine code with no I/O, threads, clocks or caches, so no Lambda or wasm fence applies; wasm32 is compile-gated in CI.
- Testing:
⚠️ The four integration tests run in CI throughgrp_misc, fail on the base, and thesort.rssite alone turns the ORDER BY assertion red. Thestage.rsfix has no test (the must-address above), and the JSON-LD twin could addgroupBy. - Conventions: ✔ Self-describing title and a thorough commit body; one sentence in the PR body about
stage.rsis inaccurate.
Verified locally at branch HEAD: cargo test -p fluree-db-api --test grp_misc -- it_decimal_exactness (all four new tests by name); all nine source files reverted to the base turns all four red; sort.rs alone turns the ORDER BY assertion red; stage.rs alone turns the INSERT…WHERE test above red.
Cross-PR note: #2010. One cross-PR note, since #2010 (mine) also changes how DISTINCT, GROUP BY and MINUS key an indexed NUM_BIG value, and I wanted to be sure it doesn't already cover this. It doesn't: with only this PR's it_decimal_exactness.rs on top of #2010, all four new tests fail, showing the "before" column of your table (DATATYPE reads xsd:decimal, DISTINCT keeps both copies). #2010 keys a NUM_BIG binding by decoding it through materialize_encoded, which is exactly the function this PR fixes, so the two are complements rather than one covering the other. Merged locally (main + this + #2010), both sides pass, 20/20 it_decimal_exactness and 25/25 of #2010's tests, with a single use-line conflict in eval/rdf.rs. So I'd merge this first, and I'll take that hunk on #2010's side.
One more overlap, with your #2017, fwiw: its formatter arm for triple-term datatypes (fluree-db-api/src/format/materialize.rs:94 on that branch) sits on the site this PR hands to resolve_dt_id_sid_for_value. I think that rule belongs inside the helper, so materialize_encoded, sort.rs and DATATYPE() get it too and not only the formatter, but both PRs are yours, so it's your call.
Approving now so you can merge without waiting on another pass from me. Just be sure the INSERT…WHERE test is in before you do, and it'd be great to take the single-graph USING fix while you're in that function.
| .dt_sids() | ||
| .get(dt_id as usize) | ||
| .cloned() | ||
| .resolve_dt_id_sid_for_value(dt_id, &other) |
There was a problem hiding this comment.
🔴 Must address before merge: this site is reachable, it was committing xsd:decimal, and nothing in the PR pins the fix.
The body says INSERT/DELETE…WHERE already behaved correctly here, so no test reaches this change. I don't think that holds for INSERT. materialize_encoded_bindings_for_txn runs on every WHERE batch when the ledger has a binary store, and a NUM_BIG EncodedLit from an indexed scan lands in this arm. Before this line it built Lit { BigInt, xsd:decimal }, and the flake generator committed that datatype.
I reverted only this file to the base and ran INSERT { ex:acct ex:copy ?o } WHERE { ex:acct ex:serial ?o } against your indexed_overflow_numerics fixture. The committed copy reads back as "123456789012345678901234567890"^^xsd:decimal, and sameTerm(?o, 123456789012345678901234567890) reports it as decimal too. At the head the same update commits xsd:integer. DELETE…WHERE is fine either way because retraction matches by value, which may be what got checked.
So this is the one site in the PR whose bug writes a wrong datatype into a committed fact instead of returning a wrong answer, and it's the only one with no test. Something like this next to the other three would cover it. I ran it: red with stage.rs at the base (left: ["xsd:decimal"]), green at the head.
#[tokio::test]
async fn indexed_overflow_integer_keeps_xsd_integer_through_insert_where() {
let (fluree, ledger) = indexed_overflow_numerics("decimal/bigint-txn:main").await;
let ledger = run_sparql_update(
&fluree,
ledger,
"PREFIX ex: <http://example.org/>
INSERT { ex:acct ex:copy ?o } WHERE { ex:acct ex:serial ?o }",
)
.await
.ledger;
let json = sparql_results(
&fluree,
&ledger,
"PREFIX ex: <http://example.org/> SELECT ?o WHERE { ex:acct ex:copy ?o }",
)
.await;
assert_eq!(binding_values(&json, "o"), vec![OVERFLOW_INT]);
assert_eq!(
binding_datatypes(&json, "o"),
vec![XSD_INTEGER],
"INSERT…WHERE copies an indexed overflow integer"
);
}It's probably worth correcting that sentence in the body too, since the merged body is where someone will look later to learn why this line changed.
There was a problem hiding this comment.
Addressed in 6a03e83 (PR body corrected too).
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn jsonld_indexed_overflow_integer_datatype_and_distinct() { |
There was a problem hiding this comment.
🟡 Optional: the JSON-LD twin could cover groupBy as well; it was visibly wrong on main.
The SPARQL tests pin DISTINCT, GROUP BY, MINUS, GRAPH and ORDER BY, and this twin pins datatype and selectDistinct. Since the fix is all shared code, I don't think JSON-LD can regress on its own. Still, groupBy is the one JSON-LD surface where the bug shows in plain to_jsonld output rather than only through @type. With the source files reverted to the base, the query below returns [[big, 1], [big, 1]]; at the head it returns [[big, 2]]. ORDER BY and GRAPH only show it through to_typed_json, so I'd skip those.
#[tokio::test]
async fn jsonld_indexed_overflow_integer_group_by() {
let (fluree, ledger) = indexed_overflow_numerics("decimal/bigint-jsonld-group:main").await;
let query = serde_json::json!({
"@context": {"ex": "http://example.org/", "xsd": "http://www.w3.org/2001/XMLSchema#"},
"select": ["?o", "(count ?o)"],
"where": [["union",
{"@id": "ex:acct", "ex:serial": "?o"},
[["values", ["?o", [{"@value": OVERFLOW_INT, "@type": "xsd:integer"}]]]]
]],
"groupBy": "?o"
});
let rows = support::query_jsonld(&fluree, &ledger, &query)
.await
.expect("groupBy query")
.to_jsonld(&ledger.snapshot)
.expect("to_jsonld");
assert_eq!(rows, serde_json::json!([[OVERFLOW_INT, 2]]), "groupBy");
}It passes at the head. This is minor and non-blocking, but if you agree it's right, I'd rather see it folded in now than lost in the backlog.
The transaction WHERE materializer decoded every encoded binding through
graph 0. NUM_BIG arena handles are numbered per graph and predicate, so
`INSERT … USING <g> WHERE { ?s ex:v ?o }` bound ?s from <g> but committed
the default graph's value for ?o. It now decodes through the WHERE's base
graph, the single resolved USING/WITH/`from` graph, or 0 otherwise.
Tests:
- INSERT…WHERE copies an indexed overflow integer as xsd:integer (pins the
stage.rs datatype fix from the previous commit, which did reach commits).
- SPARQL USING and JSON-LD `from` twins copy <g>'s value, not graph 0's.
- JSON-LD groupBy twin for the scan/VALUES unification.
2 participants
Problem
An integer beyond i64 (e.g.
123456789012345678901234567890) is stored in the NUM_BIG arena alongside big decimals. Once indexed, the scan emits it as anEncodedLitwhosedt_idisDECIMALwhatever the value, because only decoding can tellxsd:integerfromxsd:decimal. #1329 fixed two decode sites (the materializer and the API formatter). The others still took the datatype fromdt_id, so on an indexed ledger:DATATYPE(?o)(SELECT, BIND)xsd:decimalxsd:integerFILTER(DATATYPE(?o) = xsd:integer)DISTINCTover{ scan } UNION { BIND(big) }GROUP BYover the same{ BIND(big) } MINUS { scan }GRAPH <g> { … ?o }outputxsd:decimalxsd:integerORDER BY ?ooutputxsd:decimalxsd:integerCauses:
group_aggregate::materialize_encodedbuiltLit{BigInt, xsd:decimal}. That fed DISTINCT/GROUP BY/MINUS keys, so the scanned copy keyed apart from a decoded copy of the same integer. It also feeds GRAPH-exit materialization.sort.rshad its own decode with the same lookup.DATATYPE()mappeddt_idwithout decoding.Fix
BinaryIndexStore::resolve_dt_id_sid_for_value(dt_id, &val)is thedt_id-keyed twin ofresolve_datatype_sid_for_value; the decoded value's variant wins overdt_id.EncodedLitinto a typed term now goes through it, including the two Inconsistent JSON-LD rendering of xsd:decimal values: empty @type from arena decode vs plain string from flake path #1329 sites, so a new site has one helper to copy instead of several drifting versions.DATATYPE()decodes only when the binding is NUM_BIG, so other bindings keep the no-decode path.stage.rs) uses the helper too. Before this,INSERT { … ?o } WHERE { … }over an indexed overflow integer committed the copy asxsd:decimal. DELETE…WHERE was unaffected because retraction matches by value.INSERT … USING <g> WHERE { ?s ex:v ?o }bound?sfrom<g>but committed the default graph's value for?o. It now decodes through the WHERE's single default graph (USING,WITHor JSON-LDfrom). A WHERE whose default graph merges several graphs still decodes through graph 0.DictOverlay::decode_dt_sid, which resolved adt_idwithout the value.xsd:decimal, is unchanged; it stays sound becauseBigIntandDecimalkey apart.Tests
it_decimal_exactnessgains three SPARQL tests over an indexed ledger, each with a big-decimal control where one applies, and a JSON-LD twin:indexed_overflow_integer_datatype_is_xsd_integer: DATATYPE projection and FILTER.indexed_overflow_integer_unifies_with_decoded_copies: DISTINCT, GROUP BY, MINUS.indexed_overflow_integer_renders_xsd_integer_through_graph_and_order_byjsonld_indexed_overflow_integer_datatype_and_distinct:datatypebind andselectDistinctover a VALUES copy.jsonld_indexed_overflow_integer_group_by:groupByover the scanned and VALUES copies.indexed_overflow_integer_keeps_xsd_integer_through_insert_where: the committed copy isxsd:integer.insert_where_using_named_graph_copies_that_graphs_overflow_valueand its JSON-LDfromtwin: a big integer in the default graph and a big decimal in<g>. The copy is<g>'s value.All fail without the fix. Removing each fix site alone (
sort.rs,materialize_encoded,DATATYPE(),stage.rs) breaks the tests that cover it, and decoding through graph 0 again fails bothUSINGtests with the default graph's value.Related: #1329
Follow-up: #2027 (integer subtypes of overflow values still index as
xsd:integer)