fix(index): sort JSON-path values once after extraction, not the raw column#7835
fix(index): sort JSON-path values once after extraction, not the raw column#7835wjones127 wants to merge 4 commits into
Conversation
…column A JSON-path scalar index trains its target (e.g. btree) on the value extracted from the JSON column at `path`, not on the raw JSON column itself. The scanner's upstream sort only orders by the raw column, so for targets that require value-ordered input (btree, whose per-page min/max come from the first/last row), the extracted stream was not actually sorted by value. This silently corrupted page min/max stats, so range and equality queries missed rows -- particularly floats not exactly representable in float64, whose storage order diverges most from their numeric order. `JsonTrainingRequest::criteria()` now overrides the target's `Values` ordering to `None` when delegating to the scanner, since a raw-column sort can't help the extracted value anyway. `train_index` sorts the extracted `(value, row_id)` stream itself, once, right before handing it to the target trainer. Fixes lance-format#7485
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughJSON scalar index training now sorts extracted JSON-path values for value-ordered targets such as btrees. Tests cover unsorted storage input, null pairing, non-Values targets, and non-exact float predicates. ChangesJSON scalar index ordering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Scanner
participant JsonIndexPlugin
participant SortExec
participant BtreeTrainer
participant ScalarIndexQuery
Scanner->>JsonIndexPlugin: return JSON rows in scanner order
JsonIndexPlugin->>JsonIndexPlugin: extract JSON values and row ids
JsonIndexPlugin->>SortExec: sort extracted values when ordering is Values
SortExec-->>JsonIndexPlugin: return value-sorted stream
JsonIndexPlugin->>BtreeTrainer: train target index
ScalarIndexQuery->>BtreeTrainer: search float predicates
BtreeTrainer-->>ScalarIndexQuery: return matching row ids
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/python/tests/test_scalar_index.py (1)
2493-2503: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
@pytest.mark.parametrizefor the per-filter cases.These iterations differ only by the
filterinput, so they should be parametrized rather than looped. This gives each case an independent pass/fail and clearer test IDs.♻️ Suggested parametrization
`@pytest.mark.parametrize`( "filter", [ "json_get_float(data, 'latitude') > 0", "json_get_float(data, 'latitude') >= 10.5", "json_get_float(data, 'latitude') = 40.1", "json_get_float(data, 'latitude') = 10.5", "json_get_float(data, 'latitude') < 100", ], ) def test_json_index_non_exact_floats(filter): ... assert "ScalarIndexQuery" in ds.scanner(filter=filter).explain_plan() assert ds.to_table(filter=filter) == ds.to_table( filter=filter, use_scalar_index=False ), filterAs per coding guidelines: "Use
@pytest.mark.parametrizefor Python tests that differ only by inputs."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/python/tests/test_scalar_index.py` around lines 2493 - 2503, Replace the filter loop in test_json_index_non_exact_floats with a pytest.mark.parametrize decorator over the existing filter expressions, and move the shared assertions into the test body so each filter runs as an independent case with its value available through the filter parameter.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/python/tests/test_scalar_index.py`:
- Around line 2493-2503: Replace the filter loop in
test_json_index_non_exact_floats with a pytest.mark.parametrize decorator over
the existing filter expressions, and move the shared assertions into the test
body so each filter runs as an independent case with its value available through
the filter parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 02cdab06-8d53-4379-a3cf-5fe87d5793b9
📒 Files selected for processing (2)
python/python/tests/test_scalar_index.pyrust/lance-index/src/scalar/json.rs
scan_training_data only special-cases TrainingOrdering::Values (it's the only case that triggers an explicit order_by); Addresses and None both already fall through to the same unordered scan. So JsonTrainingRequest can just always report None instead of preserving Addresses/None from the target criteria. Addresses review comment on lance-format#7835.
…quest.criteria() - Remove three in-function `use` statements in the new regression test that duplicated names already in scope from the module's top-level imports. - Add a comment at the train_index ordering check clarifying it must read target_request.criteria() (the target's real criteria), not the JsonTrainingRequest's own criteria() (always None) -- so a future "simplification" doesn't silently disable the sort.
…SON index Addresses two test-coverage gaps flagged in self-review: - test_json_btree_index_null_at_path: a row missing `path` entirely (a genuine arrow-null in the extracted value column) must keep its row_id paired with its null value through sort_stream_by_value, and the resulting btree must still answer IsNull/range/equality correctly. - test_json_ngram_index_skips_value_sort: a JSON index over a target that requires TrainingOrdering::None (ngram) must skip sort_stream_by_value and still produce correct results, exercising the untested else branch in train_index. Factored the shared plugin/registry train-and-load boilerplate (now used by three tests) into train_and_load_json_index + local_json_index_store.
|
Added test coverage for two gaps found in self-review:
(Noted in the null test: an explicit JSON |
Problem
A JSON-path scalar index (
target_index_type=btree) returns wrong results when theindexed path holds float64 values that are not exactly representable (e.g.
40.1,-3.2): ranges come back empty and equality misses rows.Fixes #7485.
Root cause
The btree trainer requires its input sorted by value (page min/max are taken from the
first/last row of each page).
scan_training_datasorts by the raw JSON column whenthe target requests
TrainingOrdering::Values, butJsonIndexPluginextracts adifferent value (the value at
path) from that column downstream, so the extractedstream is not actually sorted by value. Silently corrupted page stats then cause range
and equality lookups to skip pages that hold matching rows.
Relationship to #7493, #7771, #7819
All three open PRs diagnose this correctly and fix it by adding a second
SortExecafter JSON extraction, on top of the raw-column sort the scanner already performs. That
raw-column sort is wasted work — it sorts a key nothing downstream uses.
This PR instead has
JsonTrainingRequest::criteria()override the target'sTrainingOrdering::ValuestoTrainingOrdering::Nonewhen delegating to the scanner(the scanner's sort can't help the extracted value anyway), and sorts the extracted
(value, row_id)stream once, intrain_index, right before handing it to the targettrainer. Same correctness fix, one sort instead of two.
Testing
test_json_float_btree_index_unsorted_input(rust/lance-index/src/scalar/json.rs):builds a real JSON/btree index through the plugin trainer/registry with the issue's
repro values fed in raw storage order, and asserts range/equality results across 5
cases. Also asserts
JsonTrainingRequest::criteria().ordering == TrainingOrdering::None.test_json_index_non_exact_floats(python/python/tests/test_scalar_index.py): theissue's repro end to end via
create_scalar_index+json_get_floatfilters.cargo fmt --all,cargo clippy -p lance-index --tests -- -D warnings, anduv run make lintare clean.