MOD-17912 Improve get_index performance (backport to 8.4) - #1635
Merged
Merged
Conversation
* MOD-17912 Improve `get_index` performance * fmt (cherry picked from commit 21568f4) Test adapted to the 8.4 SelectValue API: get_long/get_double/as_str are non-Option there, and JSONArrayType/get_array_type (V7 getArray, 8.8+) do not exist, so the packed-array assertion uses ijson's ArraySliceRef::is_typed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.4 #1635 +/- ##
==========================================
- Coverage 77.56% 77.01% -0.55%
==========================================
Files 16 15 -1
Lines 3945 3803 -142
==========================================
- Hits 3060 2929 -131
+ Misses 885 874 -11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of 21568f4 (#1631) to
8.4.IValue::get_indexwasarr.iter().nth(index).ArrayIterimplements onlynext(), sonth()falls back to theIteratordefault and walks the array one element at a time; on a packed typed array each skipped element is also materialized into an ownedIValue(a heap allocation). Reading a whole vector was therefore quadratic. This is the LLAPIgetAtpath RediSearch uses to ingest JSON vector fields — see RED-213492, where dim-1280 embeddings cost ~16 ms per vector and ~4,300 s of a ~4,500 s RDB load.Adaptations for
8.4:SelectValueAPI:get_long/get_double/as_strare non-Option here, andJSONArrayType/get_array_type(V7getArray, 8.8+) do not exist — the packed-array assertion uses ijson'sArraySliceRef::is_typedinstead.rust-toolchain.tomlhunk from the master commit dropped (conflicted;8.4pins channel 1.88).The fix itself is byte-identical to master; the ijson pinned here (
5676f592) has the sameArraySliceRefvariants.Test plan
cargo test -p json_path— passes (8.4)cargo fmt --checkclean🤖 Generated with Claude Code
Note
Medium Risk
Touches the JSON
get_indexpath used for vector-field ingest, so a mismatch in typed-array handling could change element values or miss indices. The change is small, localized, and covered by unit tests.Overview
Fixes quadratic
IValue::get_indexon packed typed arrays. The oldarr.iter().nth(index)walk materialized each skipped element, which made full reads of JSON vector fields (RediSearchgetAt) extremely slow.Indexing now uses
ArraySliceRefO(1)getfor heterogeneous and typed numeric slices (i8–f64). Typed elements are wrapped as ownedIValues; heterogeneous ones stay borrowed.Adds a unit test covering typed floats/longs, mixed arrays, OOB, and non-arrays.
Reviewed by Cursor Bugbot for commit 53eab8e. Bugbot is set up for automated code reviews on this repo. Configure here.