Skip to content

fix: ordering of events in a partition #8520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
oasisk merged 3 commits into from
Nov 3, 2025
Merged

fix: ordering of events in a partition #8520

oasisk merged 3 commits into from
Nov 3, 2025

Conversation

@oasisk
Copy link
Contributor

@oasisk oasisk commented Sep 18, 2025 •
edited by github-actions bot
Loading

PR Type

Bug fix, Enhancement

Description

  • Fix event ordering in fallback path

  • Enable sorting when no order specified

  • Respect ascending/descending by timestamp

  • Remove unused param underscore usage

Diagram Walkthrough

flowchart LR
  Search["search() results handling"] -- "fallback (non-merge)" --> Sort["sort_response() applied"]
  Sort -- "no order_by provided" --> Default["default to ts_column with asc/desc"]
  Sort -- "order_by provided" --> Respect["respect provided order_by fields"]
  Respect -- "sorted hits" --> Output["final response"]
  Default -- "sorted hits" --> Output
Loading

File Walkthrough
Relevant files
Bug fix
mod.rs
Correct sorting logic and default ordering                             

src/service/search/cache/mod.rs

  • Apply sort_response in single-result fallback
  • Implement default order_by from ts_column and direction
  • Use actual is_descending parameter in sorting
  • Minor refactor of parameters and comments
 +23/-14 

@github-actions github-actions bot added ☢️ Bug Something isn't working Review effort 2/5 labels Sep 18, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Sep 18, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Lifetime/Borrow Bug

The temporary vector created with '&vec![...]' is referenced by 'order_by' and then iterated in 'sort_by'; this borrows a reference to a value that is dropped at the end of the 'if' expression, potentially leading to use-after-free or borrow checker errors. Ensure 'order_by' owns the vector or restructure to avoid referencing a temporary.

 
let order_by = if in_order_by.is_empty() {
    &vec![(
        ts_column.to_string(),
        if is_descending {
            OrderBy::Desc
        } else {
            OrderBy::Asc
        },
    )]
} else {
    in_order_by
};
Unchecked Index

Directly indexing 'results[0]' without verifying that 'results' is non-empty can panic. Add a guard or handle the empty case before cloning/sorting.

 
    let mut reps = results[0].clone();
    sort_response(
        c_resp.is_descending,
        &mut reps,
        &c_resp.ts_column,
        &c_resp.order_by,
    );
    reps
    // results[0].clone()
}
Sorting Correctness

'sort_response' now defaults to sorting by 'ts_column' using 'is_descending' when 'order_by' is empty; verify this matches upstream expectations and that 'ts_column' always exists in hits. Missing fields may cause inconsistent ordering.

 
    is_descending: bool,
    cache_response: &mut search::Response,
    ts_column: &str,
    in_order_by: &Vec<(String, OrderBy)>,
) {
    let order_by = if in_order_by.is_empty() {
        &vec![(
            ts_column.to_string(),
            if is_descending {
                OrderBy::Desc
            } else {
                OrderBy::Asc
            },
        )]
    } else {
        in_order_by
    };

greptile-apps[bot]
greptile-apps bot reviewed Sep 18, 2025
View reviewed changes
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR fixes a critical ordering issue in the search cache service where events within a single partition were being returned in unpredictable chronological order. The change re-enables previously commented-out sorting logic that ensures proper ordering of cached search responses.

The fix operates at two levels:

  1. Single Partition Sorting: Re-activates the sort_response function call for single-result scenarios (lines 328-336), ensuring that even when there's only one partition, the events are properly sorted before being returned.
  2. Default Ordering Enhancement: Improves the sort_response function (lines 660-671) to provide fallback timestamp-based ordering when no explicit ORDER BY clause is specified in the query.

This addresses a gap in the search functionality where multi-partition results were properly sorted through the merge_response function, but single-partition results bypassed sorting entirely. The change is particularly important for OpenObserve's temporal data analysis capabilities, as consistent chronological ordering is essential for caching, pagination, and user experience. The enhancement ensures that all search results maintain proper ordering regardless of whether they come from single or multiple partitions, using timestamp-based sorting as a reliable fallback when no explicit ordering criteria are provided.

Confidence score: 4/5

  • This PR is safe to merge with low risk as it restores expected functionality
  • Score reflects a straightforward bug fix that re-enables existing, tested sorting logic
  • Pay close attention to the sort_response function changes to ensure default ordering logic is correct

1 file reviewed, no comments

Edit Code Review Bot Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Sep 18, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix dangling reference to temporary

Avoid returning a reference to a temporary vector that is dropped at the end of the
expression, which leads to a dangling reference and undefined behavior. Use an owned
local Vec and then work with a slice reference that lives long enough for the sort
closure. This prevents lifetime issues during sorting.

src/service/search/cache/mod.rs [660-671]

-let order_by = if in_order_by.is_empty() {
-    &vec![(
+let default_order;
+let order_by: &[(String, OrderBy)] = if in_order_by.is_empty() {
+    default_order = vec![(
         ts_column.to_string(),
-        if is_descending {
-            OrderBy::Desc
-        } else {
-            OrderBy::Asc
-        },
-    )]
+        if is_descending { OrderBy::Desc } else { OrderBy::Asc },
+    )];
+    &default_order
 } else {
-    in_order_by
+    in_order_by.as_slice()
 };
Suggestion importance[1-10]: 9

__

Why: The code returns a reference to a temporary Vec, causing a dangling reference; using an owned local and a slice fixes a critical lifetime bug accurately aligned with the diff.

High
Guard against empty results

Accessing results[0] without checking if results is non-empty risks an index
out-of-bounds panic. Guard this branch to return early or handle the empty case
safely before cloning and sorting. This prevents runtime crashes on empty result
sets.

src/service/search/cache/mod.rs [328-336]

-let mut reps = results[0].clone();
-sort_response(
-    c_resp.is_descending,
-    &mut reps,
-    &c_resp.ts_column,
-    &c_resp.order_by,
-);
+let reps = if results.is_empty() {
+    search::Response::default()
+} else {
+    let mut reps = results[0].clone();
+    sort_response(
+        c_resp.is_descending,
+        &mut reps,
+        &c_resp.ts_column,
+        &c_resp.order_by,
+    );
+    reps
+};
 reps
Suggestion importance[1-10]: 7

__

Why: Directly indexing results[0] can panic on empty input; adding a guard improves robustness, though impact depends on upstream guarantees about non-empty results.

Medium

@oasisk oasisk force-pushed the fix-ordering-of-events-in-a-partition branch from c063874 to 10946b8 Compare September 18, 2025 10:24
@oasisk oasisk force-pushed the fix-ordering-of-events-in-a-partition branch from 10946b8 to 66ac592 Compare September 19, 2025 08:53
@testdino-playwright-reporter
Copy link

testdino-playwright-reporter bot commented Nov 3, 2025

⚠️ Test Run Unstable

Author: hengfeiyang | Branch: fix-ordering-of-events-in-a-partition | Commit: 1cc2a3b

Testdino Test Results

Status Total Passed Failed Skipped Flaky Pass Rate Duration
All tests passed 376 351 0 21 4 93% 4m 31s

View Detailed Results

@testdino-playwright-reporter
Copy link

testdino-playwright-reporter bot commented Nov 3, 2025

⚠️ Test Run Unstable

Author: hengfeiyang | Branch: fix-ordering-of-events-in-a-partition | Commit: 1cc2a3b

Testdino Test Results

Status Total Passed Failed Skipped Flaky Pass Rate Duration
All tests passed 376 351 0 21 4 93% 5m 5s

View Detailed Results

@oasisk oasisk merged commit fbadbda into main Nov 3, 2025
33 of 35 checks passed
@oasisk oasisk deleted the fix-ordering-of-events-in-a-partition branch November 3, 2025 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@uddhavdave uddhavdave uddhavdave approved these changes

+1 more reviewer

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

☢️ Bug Something isn't working Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@oasisk @uddhavdave @hengfeiyang