Fix: range bound inclusivity is lost when translating to Solr - #69
Merged
Merged
Conversation
Contributor
|
Please rebase onto main to get all tests green |
`translate_opensearch_query` folded `gt` into `gte` and `lt` into `lte`, then
always emitted square brackets, so every translated range came out inclusive at
both ends. Solr supports both forms -- `[a TO b]` and `[a TO b}` -- so nothing
about the engine required this.
Effect on a shipped workload: nyc_taxis's `range` operation asks for
`{"gte": 5, "lt": 15}` and the conversion produces `total_amount:[5 TO 15]`,
which also matches `total_amount == 15`. On the 1k corpus that `--test-mode`
downloads that is 576 documents against 573; on a 300,649-document sample,
191,735 against 191,441. The same difference appears in OpenSearch itself when
its body is changed from `lt` to `lte`, so it is a property of the bound rather
than of either engine.
`test_range_query` covered only `gte`/`lte`, the one combination the old code
got right, and asserted with `assertIn("TO", …)` -- which passes for
`[5 TO 100]`, `[5 TO 100}` and `{5 TO 100}` alike. It is an assertEqual now, and
the four bound combinations plus the open-ended forms are covered. Both new
tests fail without the change.
196 unit tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
serhiy-bzhezytskyy
force-pushed
the
range-bound-inclusivity
branch
from
September 3, 2026 12:12
1923306 to
cb84d30
Compare
Contributor
Author
|
Rebased onto main — one commit, same diff. The checks still show as pending here because the workflow runs on this branch are sitting in |
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
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.
Description
_translate_query_nodefoldedgtintogteandltintolte, then always emitted square brackets:So every translated range came out inclusive at both ends, and an exclusive bound in the source workload
silently became an inclusive one. Solr supports both spellings —
[a TO b]and[a TO b}— so nothingabout the engine required this.
Effect on a shipped workload.
nyc_taxis'srangeoperation asks upstream for{"gte": 5, "lt": 15},and the conversion produces
total_amount:[5 TO 15], which also matchestotal_amount == 15:[5 TO 15]lt: 15documents-1k.json(what--test-modedownloads)The three extra documents in the 1k corpus are exactly those with
total_amount == 15:The same difference appears in OpenSearch on its own request body — changing
lttoltereturns191,735 against 191,441 — so it is a property of the bound, not of either engine.
This is not about timing: 12 runs of each query on the 1k corpus give 0–4 ms either way. It is that the
converted operation matches documents the operation it was converted from excludes, which makes a
Solr/OpenSearch comparison a comparison of two different questions. Note the direction — the conversion
matches more, so Solr does work the operation does not ask for.
The workload file that carries the current bound is fixed separately in
apache/solr-orbit-workloads;each change stands alone, and only this one keeps the next conversion from reintroducing it.
Issues Resolved
Resolves #68
Testing
New functionality includes testing
test_range_querycovered onlygte/lte— the one combination the old code got right — andasserted with
assertIn("fare_amount", …)andassertIn("TO", …), which pass for[5 TO 100],[5 TO 100}and{5 TO 100}alike. It is anassertEqualnow.Two new tests:
test_exclusive_bounds_use_curly_bracketscovers all four bound combinations, andtest_a_missing_bound_is_open_and_inclusivecovers the open-ended forms.Verified red without the fix: with
main'squery.pyrestored under the new tests, both fail —'fare_amount:[5 TO 100}' != 'fare_amount:[5 TO 100]'and'fare_amount:{5 TO *]' != 'fare_amount:[5 TO *]'. With the fix, both pass.Full unit suite:
196 passed.Worth flagging on its own: the existing test was green over this defect for two independent reasons, and
either alone was enough. It exercised the only correct combination, and its assertions could not see the
brackets even if it had exercised a wrong one.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache
2.0 license.