fix(index): convert JSON values during index updates - #8404
Conversation
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
JSON optimization needs to preserve the trained target type and JSON-wrapper semantics across both merge and append-rebuild paths, while keeping conversion streaming. A viable revision would convert each batch against the loaded target's expected type, retain the JSON path and target parameters when deriving a rebuild, and apply target ordering after extraction.
| ) -> Result<CreatedIndex> { | ||
| let target_criteria = self.target_index.update_criteria().data_criteria; | ||
| let (new_data, inferred_type) = | ||
| JsonIndexPlugin::extract_json_with_type_info(new_data, self.path.clone()).await?; |
There was a problem hiding this comment.
This newly routes every update through helpers that drain the complete delta into all_batches, then into converted_batches, before sort_stream_by_value can start its spillable SortExec. Large append optimization is therefore O(delta) memory and can OOM before spilling helps. Make extraction/conversion a batch-by-batch stream; using the target's known type removes the need for whole-delta inference.
There was a problem hiding this comment.
Addressed in 7c0b407. JSON extraction and conversion now map batches lazily; a regression verifies that the first converted batch is returned without polling the next input batch.
| let target_criteria = self.target_index.update_criteria().data_criteria; | ||
| let (new_data, inferred_type) = | ||
| JsonIndexPlugin::extract_json_with_type_info(new_data, self.path.clone()).await?; | ||
| let new_data = JsonIndexPlugin::convert_stream_by_type(new_data, inferred_type).await?; |
There was a problem hiding this comment.
This converts the delta using a type inferred only from new fragments, but the loaded target index's existing value type is the update contract. A null/missing-only delta defaults to Utf8, and an integer-only delta for a Float64 index becomes Int64; BTreeIndex::merge_segments rejects both. Use the stored target value type for conversion (and type tags for per-value validation/null handling), so compatible updates retain the established schema and incompatible drift fails with the path and expected/actual types.
Reproducer
I ran a temporary rstest that trained the initial JSON B-tree and called index.update(...) with these cases:
#[case::all_null_delta(
&[r#"{"v": 1}"#],
&[r#"{"v": null}"#, r#"{"other": 2}"#],
"Utf8 does not match segment value type Int64"
)]
#[case::integer_delta_for_float_index(
&[r#"{"v": 1.5}"#],
&[r#"{"v": 2}"#],
"Int64 does not match segment value type Float64"
)]CARGO_TARGET_DIR=/home/agent/tmp/pr8404-impl-target cargo test -p lance-index review_repro_update_infers_type_from_delta -- --nocapture
Both cases reproduced the asserted B-tree type mismatch; these updates should optimize successfully by converting to the stored Int64/Float64 type.
There was a problem hiding this comment.
Addressed in 7c0b407. Updates now convert against the B-tree's stored data type, preserve null and missing values, allow Int64 values for Float64 targets, and report the JSON path plus expected and actual types on incompatible drift. Both reproducer cases have regression coverage.
| let target_criteria = self.target_index.update_criteria(); | ||
| UpdateCriteria { | ||
| requires_old_data: target_criteria.requires_old_data, | ||
| data_criteria: json_scan_criteria(&target_criteria.data_criteria), |
There was a problem hiding this comment.
update_criteria is also consumed by rebuild_scalar_segment, so this raw-unordered scan contract reaches OptimizeOptions::append(). That path still gets direct B-tree parameters from JsonIndex::derive_index_params and passes this preprocessed raw JSON stream into B-tree training, producing BTreeIndexDetails beside existing JsonIndexDetails; the logical index then refuses to load. derive_index_params needs to preserve the JSON wrapper, path, and target parameters so rebuilds run the JSON trainer and post-extraction ordering.
Reproducer
I ran a temporary integration test with:
let initial = json_batch(vec![r#"{"val": 1000}"#]);
let appended = json_batch(vec![
r#"{"val": 3000}"#,
r#"{"val": 1000}"#,
r#"{"val": 2000}"#,
]);
dataset
.optimize_indices(&OptimizeOptions::append())
.await
.unwrap();
let error = dataset
.scan()
.filter("json_get_int(json, 'val') >= 2000")
.unwrap()
.try_into_batch()
.await
.unwrap_err();
assert!(error.to_string().contains("mixes incompatible segment types"));CARGO_TARGET_DIR=/home/agent/tmp/pr8404-impl-target cargo test -p lance review_repro_append_json_btree_index -- --nocapture
It produced one JsonIndexDetails segment and one BTreeIndexDetails segment, then failed the query with Scalar index 'json_idx' on column 'json' mixes incompatible segment types.
There was a problem hiding this comment.
Addressed in 7c0b407. JsonIndex::derive_index_params now retains the JSON wrapper, path, target type, and target parameters; merge and append-rebuild integration cases both pass.
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The direct-update path is fixed, but append rebuilds still need to preserve the loaded JSON target learned value type, not only the wrapper and construction parameters. Carry that type through derived rebuild parameters and convert against it so every segment remains query-compatible.
| target_index_type: target_params.index_type, | ||
| target_index_parameters: target_params.params, | ||
| path: self.path.clone(), | ||
| }; |
There was a problem hiding this comment.
These derived parameters still drop the learned target value type. OptimizeOptions::append() rebuilds the new segment from them, so a Float64 base followed by an integer-only delta is inferred as Int64. Both segments still load as JSON, but the Float64 query is evaluated against Int64 B-tree page statistics and silently prunes the appended row. Include the trained target type in the derived JSON parameters (optionally absent for an initial build) and make rebuild training convert against it instead of inferring per segment.
Reproducer
I ran a disposable end-to-end regression that trained a JSON B-tree from {"val":1.5}, appended {"val":2}, and completed OptimizeOptions::append(). The same json_get_float equality predicate on path val for 2.0 returned one row with scalar indexes disabled but zero rows with the index enabled; the indexed assertion failed with left: 0, right: 1.
CARGO_TARGET_DIR=/home/agent/tmp/pr8404-followup-target cargo test -p lance test_disposable_optimize_append_float_json_with_integer_delta -- --nocapture
The command exited 101 with 0 passed and 1 failed.
There was a problem hiding this comment.
Addressed in 61fccdf. Derived JSON parameters now include the optional learned target data type, and rebuild training converts each new segment against that type instead of re-inferring it. The Float64-base/Int64-delta append regression now verifies indexed results against an index-disabled baseline.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The learned JSON B-tree type is now preserved through append rebuild parameters, so direct updates and new segments share the same schema while initial builds retain inference. The prior Float64-base/integer-append false negative now passes against an index-disabled baseline, and conversion remains streaming.
Summary
Root cause
Initial JSON index training converted the selected path from raw JSONB into its inferred scalar type, but the JSON index update path forwarded newly appended raw
LargeBinaryJSON directly to the target B-tree. Optimizing the index then tried to merge that raw stream with the existing typedInt64index data.Validation
cargo test -p lance-index scalar::json::tests -- --nocapturecargo test -p lance test_optimize_json_btree_index -- --nocapturecargo fmt --all -- --checkcargo clippy --all --tests --benches -- -D warningsFixes #5177