Skip to content

Render Redis numeric range leaves as bounded intervals - #336

Merged
fcostaoliveira merged 2 commits into
masterfrom
fix/redis-bounded-range
Oct 23, 2026
Merged

fcostaoliveira merged 2 commits into
masterfrom
fix/redis-bounded-range

Conversation

@fcostaoliveira

@fcostaoliveira fcostaoliveira commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

A two-sided numeric filter such as 0 < price < 3 currently reaches Redis as two open-ended ranges joined by an intersection. OpenSearch receives one bounded range. Render each canonical range leaf as bounded intervals, preserving inclusive/exclusive endpoints, parameter conversion, and separate boolean leaves. Multiple same-side constraints retain every lower/upper pairing.

This also fixes multivalued semantics: a document with price = [-10, 10] must not match 0 < price < 3. Split predicates could match different values; a bounded interval requires one value to satisfy the entire leaf. Single-valued numeric datasets retain the same matches. No performance improvement is claimed until a controlled rerun completes.

Redis and Dragonfly, which shares the parser, record numeric_range_encoding: bounded_leaf_v1 after successful filter resolution and before timing. The metadata describes prepared encoding, not executed query counts under query truncation. Valkey's independent parser is outside this change.

Validation:

  • make agent-check: 1,207 tests, format and clippy checks pass.
  • Redis integration: 47 tests pass. The real CLI regression captures FT.SEARCH, replays it byte-for-byte, checks exact IDs/counts, and requires one NUMERIC iterator. Restoring split queries makes it fail. Unit guards also fail under a split-renderer mutation; range tests passed in debug and release.
  • Dragonfly integration: 15 tests pass. Its real CLI bounded datetime-range fixture verifies provenance; a tag-only fixture verifies no range marker. The new provenance assertion failed before adding the Dragonfly hook.
  • Python v0 and Rust agree at 1.0 recall for all 18 full-width Redis boundary queries. The multivalue oracle independently checks same-value interval semantics across endpoint ties, reversed ranges, and all bound combinations.

@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

🤖 Automated first-pass review. A human maintainer still needs to review this before merge.

What I checked: the new renderer (src/redis_numeric_range.rs), the build_range_filter rewrite and the provenance hook in redis.rs, every caller of redis::parse_conditions in the tree (Dragonfly goes through it via dragonfly::parse_conditions; Valkey, KiviDB and VectorSets each have their own builder, so none of them change here), the param-naming scheme the suffix check relies on, and the two integration tests. I found no correctness, isolation or fail-loud defects. The #219 behaviour still holds: a leaf whose bounds all fail to parse still gets None from render(&[]), so the "filter declared but unbuildable" path filter_guard.rs:276 describes is still reachable and still a hard error. only_emitted_range_parameters_identify_range_encoding pins that case for both the old and new shapes.

Worth fixing before merge

Nothing.

The PR already does the work I'd otherwise ask for:

  • An exhaustive multivalue oracle: 3⁴ endpoint values × 15 bound masks × 4 documents. It covers ties, reversed ranges and mixed inclusivity, and it parses the emitted syntax independently of the renderer.
  • A real-CLI regression that captures the actual FT.SEARCH from SLOWLOG, replays it byte for byte, checks exact IDs rather than counts, and asserts a single NUMERIC with no INTERSECT in FT.EXPLAIN. It runs for both the KNN and --skip-vector-index paths.
  • A mutation claim for the split renderer.

The gte:3, lte:0 case (integration_redis.rs:700) is a useful addition: it confirms a reversed interval returns empty from the server rather than erroring.

Optional / follow-up

  1. Valkey is now on a different range encoding from Redis. valkey.rs:1133 keeps the split [-inf ($lt] [$gte +inf] form. On today's hash-backed, single-valued corpora the match sets are identical, so recall is unaffected. However, any Redis vs Valkey filtered-latency comparison now also compares query encodings (one bounded iterator against an intersection of two open-ended scans). The PR body scopes this out explicitly, which is fine. It's worth filing as a tracked parity issue so it isn't lost, and so a reader of a cross-engine table knows Valkey results carry no numeric_range_encoding marker for a reason other than "no ranges".
  2. is_bound_parameter depends on a naming convention, not a type. It's sound against the current builders: every non-range param ends in the counter digit (redis.rs:1313/1328/1337/1362/1405) or in lon/lat/radius. But a future builder that names a param …_lt/…_gt would turn the provenance marker on silently. The failure mode here is a wrong telemetry string, not a wrong result, so this isn't a blocker. If you want it structural, build_range_filter could report "emitted a bound" alongside the ParsedFilter rather than having the marker re-derived from PARAMS keys afterwards.
  3. Small fragility in the integration test. FT.EXPLAIN hard-codes DIALECT 2/4 (integration_redis.rs:806) rather than taking it from the captured command. If the driver's dialect ever changes, the explain could stop matching the search it claims to describe. Reading DIALECT out of args would keep the two in step.
  4. On "no performance improvement is claimed until a controlled rerun completes": agreed, and thanks for holding that line. When the rerun happens, a 1-vs-2-iterator A/B on one engine version, with the encoding as the only variable, is the table that would settle it.

Net: a small, well-isolated encoding change. The semantics argument is sound, the oracle is independent, a live replay backs it, and the provenance marker is recorded before timing. Nothing blocks merge. The Valkey divergence is the one item that deserves an issue rather than silence.

@fcostaoliveira

Copy link
Copy Markdown
Collaborator Author

Addressed the Dragonfly provenance finding in 0dde6f9. Dragonfly now invokes the same recording hook immediately after its filter-resolution step. The real CLI datetime-range integration fixture checks bounded_leaf_v1; a tag-only fixture checks that the marker stays absent.

Validation: make agent-check passed (1,207 tests plus format/clippy), make integration-test passed all 47 Redis tests, and make integration-test-dragonfly passed all 15 tests. The new Dragonfly assertion was first run against the old path: 14 passed and the datetime test failed because provenance was null. Adding the hook made that guard pass. CI is running again for the new commit.

The PR description now explicitly identifies the multivalued wrong-answer correction and states that no performance improvement is claimed. Valkey's independent parser remains outside this change; follow-up #337 tracks support verification, canonical same-value semantics, real request/result tests, and provenance. Redis/Valkey two-sided-range results should not be presented as identical query shapes before that follow-up.

The parameter-name convention is covered by the existing positive/negative unit guard; structural provenance can be a follow-up if parser metadata evolves. src/redisearch/parser.rs is not exported by the library or imported by the current binary, so it is not used by this harness or the v0/Rust comparison.

@fcostaoliveira

Copy link
Copy Markdown
Collaborator Author

Disposition of the remaining optional suggestions: Valkey encoding parity is tracked in #337. Typed parser provenance and deriving FT.EXPLAIN's DIALECT from the captured FT.SEARCH request are tracked in #338, with regression-test expectations. Both are future drift prevention; the existing naming convention and dialects match current requests. Keeping this patch stable while its final CI completes.

@github-actions

Copy link
Copy Markdown

Docker Build Validation

Platforms tested:

  • linux/amd64 (built and tested)
  • linux/arm64 (build validated)

Tests performed:

  • --help command
  • --describe datasets command
  • Integration test (random-100 dataset + Redis)
  • Multi-platform build validation (amd64 + arm64)

1 similar comment
@github-actions

Copy link
Copy Markdown

Docker Build Validation

Platforms tested:

  • linux/amd64 (built and tested)
  • linux/arm64 (build validated)

Tests performed:

  • --help command
  • --describe datasets command
  • Integration test (random-100 dataset + Redis)
  • Multi-platform build validation (amd64 + arm64)

@fcostaoliveira
fcostaoliveira merged commit d3f4cc3 into master Oct 23, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant