Greptile Overview

Greptile Summary

This PR adds caching support for histogram queries with non-timestamp column ORDER BY (e.g., ORDER BY count DESC). When such queries are detected, the cache system scans all hits to find min/max timestamps instead of using the fast first/last optimization.

Key Changes:

• Added is_histogram_non_ts_order flag to CacheQueryRequest to track histogram queries ordered by non-timestamp columns
• Created utility functions is_timestamp_field(), has_non_timestamp_ordering(), and extract_timestamp_range() in result_utils.rs
• Modified cache check logic in cacher.rs to allow histogram queries even when ORDER BY is not on timestamp
• Updated write_results() to scan all hits for time range when is_histogram_non_ts_order=true
• Propagated flag through both standard and streaming cache write paths

Critical Issue Found:

• Logic inconsistency between cache check path (cacher.rs:213-215) and cache write paths (mod.rs:427-433, streaming/cache.rs:84-90)
• Cache check uses has_non_timestamp_ordering() which checks if ANY ORDER BY field is non-timestamp (.any())
• Cache write paths only check if the FIRST ORDER BY field is non-timestamp (.first())
• For multi-column ORDER BY like ORDER BY _timestamp, count DESC, this creates a mismatch that could cause incorrect caching behavior

Confidence Score: 2/5

• This PR has a critical logic bug that will cause cache inconsistencies for multi-column ORDER BY clauses
• The inconsistent logic between cache check and cache write paths is a blocking issue. While the overall approach is sound, the .any() vs .first() mismatch in ORDER BY field checking will cause the cache check to flag queries as non-time-ordered when they're actually time-ordered (if timestamp is first), leading to unnecessary full-scan overhead and potential cache key mismatches
• Pay close attention to src/service/search/cache/cacher.rs and src/service/search/cache/result_utils.rs - the logic bug must be fixed before merge

Important Files Changed

File Analysis

Sequence Diagram

sequenceDiagram
    participant Client
    participant Cacher as cache/cacher.rs
    participant Utils as cache/result_utils.rs
    participant Mod as cache/mod.rs
    participant Stream as streaming/cache.rs
    participant Disk as Cache Storage

    Client->>Cacher: check_cache()
    Cacher->>Cacher: Detect is_histogram_query
    Cacher->>Utils: has_non_timestamp_ordering(order_by, ts_col)
    Note over Utils: BUG: Uses .any() checks ALL fields
    Utils-->>Cacher: is_histogram_non_ts_order
    Cacher->>Cacher: Set flag in CacheQueryRequest
    Cacher->>Disk: Check for cached results
    
    alt Cache Miss - Execute Query
        Cacher-->>Mod: Execute query
        Mod->>Mod: Get results with order_by_metadata
        Mod->>Mod: Compute is_histogram_non_ts_order
        Note over Mod: Only checks FIRST field (.first())
        Mod->>Utils: extract_timestamp_range(hits, is_time_ordered)
        Utils->>Utils: Scan all hits if !is_time_ordered
        Utils-->>Mod: (min_ts, max_ts)
        Mod->>Disk: write_results(is_histogram_non_ts_order)
    else Streaming Path
        Stream->>Stream: Compute is_histogram_non_ts_order
        Note over Stream: Only checks FIRST field (.first())
        Stream->>Mod: write_results()
        Mod->>Utils: extract_timestamp_range()
        Utils-->>Mod: (min_ts, max_ts)
        Mod->>Disk: write_results()
    end
    
    Note over Cacher,Stream: INCONSISTENCY: cacher uses .any(), mod/stream use .first()

_{5 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}