Repository navigation
Conversation
record_new_worst_hit rounded the worst top-K hit up to the next second. Split time ranges are inclusive whole seconds, so a split ending in the same second as the worst hit can still hold a better document. Rounding up discarded those splits and could return the wrong top K. Round down instead (div_euclid, for both sort orders).
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6067bfd6dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fmassot
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fcc3dc593
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Newly indexed splits round their maximum timestamp up to whole seconds. | ||
| // For a worst hit at 1.5s, a split can do better only if its upper bound | ||
| // is at least 2s. Legacy truncated bounds can still cause false pruning. |
There was a problem hiding this comment.
For indexes containing splits created before this upgrade, the maximum timestamp remains truncated toward zero, and merge_time_range deliberately preserves that legacy bound. With a positive-time split whose recorded end is 1601 but which contains a document at 1601.9s, a current worst hit at 1601.758s still produces threshold 1602, so can_be_better skips the split and returns an incorrect top K. This affects existing indexes and mixed-version deployments—the common upgrade path—not merely historical edge cases; use a threshold compatible with legacy bounds or disable this pruning for legacy metadata.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think we are fine with that
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dcd4edcf3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .div_euclid(1_000_000_000); | ||
| let end = quickwit_common::div_ceil(range.end().into_timestamp_nanos(), 1_000_000_000); |
There was a problem hiding this comment.
When a timestamp reaches DateTime's nanosecond limits, this rounding stores -9_223_372_037 or 9_223_372_037 seconds, as the new extreme-value test explicitly permits. merge_time_range later passes those bounds to DateTime::from_timestamp_secs, but neither whole-second value fits in its i64 nanosecond representation, so a later merge can panic on overflow or reconstruct a wrapped, invalid range. Convert these metadata bounds back with saturating nanosecond multiplication (or equivalent clipping) before creating the merged DateTime range.
Useful? React with 👍 / 👎.
This branch has not been deployed
2 participants
Problem
For a timestamp-descending top-10 search:
Sorting splits does not prevent this: several splits can have the same truncated maximum.
Fix
As suggested by @PSeitz, store split bounds as floor(min)..=ceil(max). In the example, the upper bound becomes 1602, so the split is searched. Search logic and metadata format/version stay unchanged.
Newly indexed splits only: old metadata is not repaired, and merges inheriting old bounds can retain the bug. Overflow handling at timestamp limits during merges is left for a follow-up.
Tests
Metadata rounding and same-second pruning (descending, ascending, trace IDs), including negative timestamps.
make fmtandgit diff --checkpassed. Indexing/search crate tests: 458 passed, 1 skipped. The previous 1B-document benchmark was not rerun.