Skip to content

Conversation

@xav-db
Copy link
Member

@xav-db xav-db commented Feb 8, 2026

Greptile Overview

Greptile Summary

Fixed a critical Ord trait violation in the Value enum's comparison implementation. Previously, incomparable types (e.g., Date vs non-parseable String, or mismatched variants) returned Ordering::Equal, which violates Rust's Ord contract since a.cmp(b) == Equal should only occur when a == b.

Changes:

  • Added variant_order() helper method that assigns a deterministic u8 ordering value to each Value variant
  • Replaced Ordering::Equal fallback cases with self.variant_order().cmp(&other.variant_order())
  • Ensures total ordering across all Value types, preventing potential undefined behavior in sorting algorithms and data structures

Impact:
This fix prevents subtle bugs in any code that relies on sorting or ordering Value types, particularly when dealing with heterogeneous data.

Important Files Changed

Filename Overview
helix-db/src/protocol/value.rs Fixed Ord trait violation by using variant ordering instead of Equal for incomparable types. Added variant_order() helper method for consistent type ordering.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Value_Ord
    participant variant_order
    
    Caller->>Value_Ord: cmp(self, other)
    
    alt Same type comparison
        Value_Ord->>Value_Ord: Direct comparison (String, Date, etc.)
        Value_Ord-->>Caller: Ordering result
    else Integer types
        Value_Ord->>Value_Ord: Convert to i128 and compare
        Value_Ord-->>Caller: Ordering result
    else Date ↔ String
        Value_Ord->>Value_Ord: Try parse String as Date
        alt Parse successful
            Value_Ord->>Value_Ord: Compare dates
            Value_Ord-->>Caller: Ordering result
        else Parse failed (NEW)
            Value_Ord->>variant_order: Get variant order for self
            Value_Ord->>variant_order: Get variant order for other
            variant_order-->>Value_Ord: u8 ordering values
            Value_Ord->>Value_Ord: Compare u8 values
            Value_Ord-->>Caller: Deterministic ordering
        end
    else Incomparable types (NEW)
        Value_Ord->>variant_order: Get variant order for self
        Value_Ord->>variant_order: Get variant order for other
        variant_order-->>Value_Ord: u8 ordering values
        Value_Ord->>Value_Ord: Compare u8 values
        Value_Ord-->>Caller: Deterministic ordering
    end
Loading

… Value types

The Ord impl for Value was returning Ordering::Equal for cross-type
comparisons (e.g. Date vs unparsable String, or any mismatched types).
This violated the Ord contract since PartialEq correctly returned false
for these pairs. Using Equal when values aren't equal breaks BTreeMaps,
sorting stability, and deduplication.

Now falls back to comparing variant discriminants, which gives a
consistent total order without falsely claiming equality.

https://claude.ai/code/session_01MYGPgMX6xJmjz7g27bsMcD
@xav-db xav-db changed the base branch from main to dev February 8, 2026 10:20
@xav-db xav-db changed the base branch from dev to hql-improvments February 8, 2026 10:21
@xav-db xav-db changed the title fix(analyzer): add E659 type check for non-boolean WHERE clause expressions WHERE clauses that contain traversals returning nodes/edges instead of a boolean were silently accepted. Now the analyzer checks that thetraversal's last step is a BoolOp, and emits E659 with a hint to wrapwith EXISTS(...) when it isn't. fix(core): date parsing ordering violation Feb 8, 2026
@xav-db
Copy link
Member Author

xav-db commented Feb 8, 2026

@greptile

The test was asserting Ordering::Equal for mixed-type comparisons,
which was the exact bug the previous commit fixed. Updated to assert
consistent antisymmetric ordering and non-equality instead.

https://claude.ai/code/session_01MYGPgMX6xJmjz7g27bsMcD
@xav-db xav-db merged commit a3bfd7b into hql-improvments Feb 8, 2026
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