Skip to content

fix(local): skip bool payload values in order_by to match server semantics - #1416

Merged
joein merged 2 commits into
qdrant:devfrom
feiiiiii5:fix/local-order-by-skip-bool
Sep 10, 2026
Merged

fix(local): skip bool payload values in order_by to match server semantics#1416
joein merged 2 commits into
qdrant:devfrom
feiiiiii5:fix/local-order-by-skip-bool

Conversation

@feiiiiii5

Copy link
Copy Markdown

Fixes #1415

All Submissions:

Changes to Core Features:

  • Explanation below; [x] new regression tests; [x] ran tests locally (see Test)

Root Cause

to_order_value (qdrant_client/local/order_by.py) filters with isinstance(value, (int, float)). In Python bool subclasses int, so True/False pass the filter and are stored as the record's order_value in _scroll_by_value. But Record.order_value is OrderValue = StrictInt | StrictFloat, which rejects bools at validation - so any bool point on the sort key crashes the whole local scroll with a pydantic ValidationError (pre-fix proof below), instead of being skipped.

The server can never produce a bool ordering value: OrderValue::try_from(Value) (qdrant/lib/segment/src/data_types/order_by.rs) only accepts as_i64() / as_f64() (both None for Bool per serde_json docs), and both order-by read paths (lib/segment/src/segment/read_view/order_by.rs: filtered_read_by_index_ordered / filtered_read_by_value_stream) read exclusively from numeric_index_for(&order_by.key) - bool payloads live in the bool index and yield zero ordering values. Same root cause as #1259 (filters), #1389 (facet), #1413/#1414 (grouping); this is the last isinstance(..., (int, ...)) site in local mode that admits bools.

Fix

Bool guard at the top of to_order_value, mirroring the check_range convention in payload_filters.py:

if isinstance(value, bool):
    return None
if isinstance(value, (int, float)):
    return value

Bool points then carry no ordering value and are skipped. start_from needs no change: it arrives through the validated StartFrom = StrictInt | StrictFloat | datetime | date model, which already rejects bools; only raw payload values bypass validation.

Test

  • New pure-local regression suite qdrant_client/local/tests/test_order_by.py (unit: to_order_value(True/False) is None, int/float/None unchanged; end-to-end: mixed bool/int scroll asc skips bools [(4, 0), (2, 1)], desc [(2, 1), (4, 0)])
  • pre-fix FAIL proof (unmodified 7ae6202): 3 failed - unit assertion plus both scroll tests crashing with ValidationError: Input should be a valid integer [input_value=True, input_type=bool]
  • post-fix: new suite 3 passed; full qdrant_client/local/tests/ 83 passed; tests/test_in_memory.py 4 passed
  • ruff format --check --line-length=99 clean on both files (repo-pinned 0.4.3 config; checked with system ruff 0.16.6)

Diff scope

2 files, +66/-0 (core 1 file, +6/-0 incl. comments; test 1 file, +60/-0)

Not PR-related failures

None encountered; no server-backed suites run (pure-local change).

@netlify

netlify Bot commented Sep 10, 2026

Copy link
Copy Markdown

Deploy Preview for poetic-froyo-8baba7 ready!

Name Link
🔨 Latest commit c110962
🔍 Latest deploy log https://app.netlify.com/projects/poetic-froyo-8baba7/deploys/6aa27726f910690008cde732
😎 Deploy Preview https://deploy-preview-1416--poetic-froyo-8baba7.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: efbbcba8-7de9-4c6e-a9fa-9086f5d9d223

📥 Commits

Reviewing files that changed from the base of the PR and between 876c3b8 and c110962.

📒 Files selected for processing (2)
  • qdrant_client/local/order_by.py
  • tests/congruence_tests/test_scroll_order_by.py
💤 Files with no reviewable changes (1)
  • qdrant_client/local/order_by.py

Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

The local order-by conversion retains the boolean guard that excludes boolean payloads from ordering values. The congruence tests add mixed boolean and integer payloads, create an integer index, verify point storage, and compare scroll results across gRPC, HTTP, and local clients.

Estimated code review effort: 2 (Simple) | ~10 minutes

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to c1109

The change skips boolean payloads during local order-by conversion, preventing the reported scrolling failure. Mixed-value cross-client coverage is supplied, so the change is ready to merge with normal checks.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The regression test addresses issue #1415, but the summarized implementation only removes a comment and leaves the boolean handling unchanged. The required boolean guard in to_order_value is not prese… Add an isinstance(value, bool) guard before the integer/float check in to_order_value, return None for booleans, and verify the regression tests pass with the guard applied.
Docstring Coverage ⚠️ Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the local order_by boolean-handling fix and matches the pull request objective.
Description check ✅ Passed The description explains the boolean ordering bug, the intended fix, and the regression tests. It is related to the changeset.
Out of Scope Changes check ✅ Passed The changes are limited to order_by behavior documentation and regression coverage. No unrelated code changes are identified.
Full details: Linked Issues check

Explanation

The regression test addresses issue #1415, but the summarized implementation only removes a comment and leaves the boolean handling unchanged. The required boolean guard in to_order_value is not present, so local scrolling can still fail on boolean payload values.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@joein
joein self-requested a review September 10, 2026 09:18
@joein
joein merged commit b29b638 into qdrant:dev Sep 10, 2026
8 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.

2 participants