fix(engine): Fix bug in SortMerge which caused rows to be skipped due to incorrect sorting - #18334
Merged
Conversation
This will cause the tests to fail because of an incorrect
implementation:
```console
$ gotest -v ./pkg/engine/executor/... -test.run=TestSortMerge
=== RUN TestSortMerge
=== RUN TestSortMerge/invalid_column_name
=== RUN TestSortMerge/ascending_timestamp
sortmerge_test.go:89:
Error Trace: /home/christian/sandbox/grafana/loki/pkg/engine/executor/sortmerge_test.go:89
Error: Should be true
Test: TestSortMerge/ascending_timestamp
Messages: timestamps are not sorted in ASC order: [1000000000000001 1000000001000001 1000000002000000 1000000002000001 1000000003000000 1000000004000000 1000003000000001 1000003001000001 1000003000000000 1000003001000000 1000003002000000 1000003002000000 1000003002000001 1000003003000000 1000003004000000 1000006000000001 1000006001000001 1000006000000000 1000006001000000 1000006002000000 1000006002000000 1000006002000001 1000006003000000 1000006004000000 1000009000000001 1000009000000000 1000009001000000 1000009002000000 1000009002000000 1000012000000000]
=== RUN TestSortMerge/descending_timestamp
sortmerge_test.go:136:
Error Trace: /home/christian/sandbox/grafana/loki/pkg/engine/executor/sortmerge_test.go:136
Error: Should be true
Test: TestSortMerge/descending_timestamp
Messages: timestamps are not sorted in DESC order: [1000003000000000 1000002999000000 1000002998000000 1000000002000000 1000000001000000 1000000000000001 1000000000000000 1000000000000000 999999999000001 999999998000001 999999999000000 999999998000000 999997002000000 999997001000000 999997000000001 999997000000000 999997000000000 999996999000001 999996998000001 999996999000000 999996998000000 999994002000000 999994001000000 999994000000001 999994000000000 999994000000000 999993999000001 999993998000001 999991002000000 999991000000001]
--- FAIL: TestSortMerge (0.00s)
--- PASS: TestSortMerge/invalid_column_name (0.00s)
--- FAIL: TestSortMerge/ascending_timestamp (0.00s)
--- FAIL: TestSortMerge/descending_timestamp (0.00s)
FAIL
FAIL github.com/grafana/loki/v3/pkg/engine/executor 0.011s
FAIL
```
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
This was referenced Nov 3, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
This PR fixes two problems with the sorting of rows of input records:
batchIndexes []int) based on their current timestamp at indexoffset(timestamps []int64), the positional correspondence was not maintained leading to pull rows from wrong records.Fixing the assertion in the testcases made these problems visible.