[new feature] logql:support sort and sort_desc #7989

liguozhong · 2022-12-21T09:23:23Z

What this PR does / why we need it:

sort()
sort(v instant-vector) returns vector elements sorted by their sample values, in ascending order.

sort_desc()
Same as sort, but sorts in descending order.

ex:
sort(rate(({app=~"foo|bar"} |~".+bar")[1m])) without (app)

prometheud doc: https://prometheus.io/docs/prometheus/latest/querying/functions/#sort

Which issue(s) this PR fixes:
Fixes #7933

Special notes for your reviewer:

Checklist

Reviewed the CONTRIBUTING.md guide
Documentation added
Tests updated
CHANGELOG.md updated
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

grafanabot · 2022-12-21T11:19:57Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

dannykopping

This is awesome, thanks @liguozhong!!

dannykopping · 2022-12-21T13:55:59Z

pkg/logql/engine.go

-		sort.Slice(vec, func(i, j int) bool { return labels.Compare(vec[i].Metric, vec[j].Metric) < 0 })
+		sortByValue, err := Sortable(q.params)
+		if err != nil {
+			return nil, err


Wrap this error please

done，thanks

dannykopping · 2022-12-21T13:56:55Z

pkg/logql/engine.go

+		if err != nil {
+			return nil, err
+		}
+		if !sortByValue {


I'm confused; why is this !sortByValue?

sort.Slice(vec, func(i, j int) bool { return labels.Compare(vec[i].Metric, vec[j].Metric) < 0 })
Because this line of code exists before this PR, the meaning of this line of code seems to be sortByLabel(), but sort and sort_desc need sortByValue(), so when we have sort and sort_desc in our logql grammar, we should not execute sortByLabel ()

The purpose of modifying this line of code is to make this test pass.
I debugged for a long time at the beginning, and it was correctly sorted by value in the front, but the final output is indeed in the string order of the label.

//sort and sort_desc { `sort(rate(({app=~"foo|bar"} |~".+bar")[1m])) without (app) + 1`, time.Unix(60, 0), logproto.FORWARD, 100, [][]logproto.Series{ { newSeries(testSize, factor(10, identity), `{app="foo"}`), newSeries(testSize, offset(46, identity), `{app="bar"}`), newSeries(testSize, factor(5, identity), `{app="fuzz"}`), newSeries(testSize, identity, `{app="buzz"}`), }, }, []SelectSampleParams{ {&logproto.SampleQueryRequest{Start: time.Unix(0, 0), End: time.Unix(60, 0), Selector: `rate({app=~"foo|bar"}|~".+bar"[1m])`}}, }, promql.Vector{ promql.Sample{Point: promql.Point{T: 60 * 1000, V: 1.1}, Metric: labels.Labels{labels.Label{Name: "app", Value: "foo"}}}, promql.Sample{Point: promql.Point{T: 60 * 1000, V: 1.2}, Metric: labels.Labels{labels.Label{Name: "app", Value: "fuzz"}}}, promql.Sample{Point: promql.Point{T: 60 * 1000, V: 1.25}, Metric: labels.Labels{labels.Label{Name: "app", Value: "bar"}}}, promql.Sample{Point: promql.Point{T: 60 * 1000, V: 2}, Metric: labels.Labels{labels.Label{Name: "app", Value: "buzz"}}}, }, },

adityacs · 2022-12-25T14:56:58Z

pkg/logql/engine.go

+			return nil, err
+		}
+		if !sortByValue {
+			sort.Slice(vec, func(i, j int) bool { return labels.Compare(vec[i].Metric, vec[j].Metric) < 0 })


I may be wrong. Just to clarify though. Should this not be sorted on V like below ?

sort.Slice(vec, func(i, j int) bool { return vec[i].V < vec[j].V })

Because this line of code exists before this PR, the meaning of this line of code seems to be sortByLabel(), but sort and sort_desc need sortByValue(), so when we have sort and sort_desc in our logql grammar, we should not execute sortByLabel ()

sortByValue is here to complete the sorting, and only here can the syntax tree be correctly parsed to accurately locate the need for sort or sort_desc.

} else if expr.Operation == syntax.OpTypeSortDesc { result[groupingKey].heap = make(vectorByValueHeap, 0) heap.Push(&result[groupingKey].heap, &promql.Sample{ Point: promql.Point{V: s.V}, Metric: s.Metric, }) } else if expr.Operation == syntax.OpTypeSort { result[groupingKey].reverseHeap = make(vectorByReverseValueHeap, 0) heap.Push(&result[groupingKey].reverseHeap, &promql.Sample{ Point: promql.Point{V: s.V}, Metric: s.Metric, }) }

Thank you. My bad, I didn't see this code earlier.

Thanks for taking the time to review this PR，👍

grafanabot · 2022-12-26T06:12:40Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

owen-d

I've been looking forward to sort in Loki for a long time! Great start, I just have a few notes:

I think we need to prevent by|without groupings on sort operations. This will keep compatibility with promql and allowing sort by (foo) doesn't make much sense anyway when sort orders by value instead of labels.

Some small changes and this will be ready to merge :)

pkg/logql/evaluator.go

grafanabot · 2023-01-04T06:03:29Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

owen-d

I've left one suggestion, then LGTM!

pkg/logql/syntax/parser.go

Co-authored-by: Owen Diehl <ow.diehl@gmail.com>

grafanabot · 2023-01-04T15:15:26Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

SHaaD94 · 2023-02-18T04:21:49Z

Hi guys.

What is the use case for this sort function?
API already has a direction parameter

liguozhong added 2 commits December 21, 2022 17:15

[new feature] logql:support sort and sort_desc

698f10c

add sort_desc test

cc3bcad

liguozhong requested a review from a team as a code owner December 21, 2022 09:23

pull-request-size bot added the size/L label Dec 21, 2022

liguozhong added 2 commits December 21, 2022 17:27

Merge branch 'main' into logql_sort

15ae9ce

add doc

656e1e6

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Dec 21, 2022

liguozhong added 4 commits December 21, 2022 17:33

add changelog

6092438

trigger ci

9a96798

ci

86286e2

ci

401a472

dannykopping reviewed Dec 21, 2022

View reviewed changes

adityacs reviewed Dec 25, 2022

View reviewed changes

liguozhong added 2 commits December 26, 2022 14:04

Merge branch 'main' into logql_sort

e0ed49c

wrap error

1b77af4

owen-d requested changes Jan 3, 2023

View reviewed changes

pkg/logql/evaluator.go Show resolved Hide resolved

prevent by|without groupings on sort operations

d62a1c5

owen-d reviewed Jan 4, 2023

View reviewed changes

pkg/logql/syntax/parser.go Outdated Show resolved Hide resolved

Update pkg/logql/syntax/parser.go

84d362a

Co-authored-by: Owen Diehl <ow.diehl@gmail.com>

owen-d approved these changes Jan 4, 2023

View reviewed changes

owen-d merged commit b444091 into grafana:main Jan 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[new feature] logql:support sort and sort_desc #7989

[new feature] logql:support sort and sort_desc #7989

liguozhong commented Dec 21, 2022 •

edited

Loading

grafanabot commented Dec 21, 2022

dannykopping left a comment

dannykopping Dec 21, 2022

liguozhong Dec 26, 2022

dannykopping Dec 21, 2022

liguozhong Dec 26, 2022

liguozhong Dec 26, 2022 •

edited

Loading

adityacs Dec 25, 2022 •

edited

Loading

liguozhong Dec 26, 2022

adityacs Dec 27, 2022

liguozhong Dec 27, 2022

grafanabot commented Dec 26, 2022

owen-d left a comment •

edited

Loading

grafanabot commented Jan 4, 2023

owen-d left a comment

grafanabot commented Jan 4, 2023

SHaaD94 commented Feb 18, 2023 •

edited

Loading

[new feature] logql:support sort and sort_desc #7989

[new feature] logql:support sort and sort_desc #7989

Conversation

liguozhong commented Dec 21, 2022 • edited Loading

grafanabot commented Dec 21, 2022

dannykopping left a comment

Choose a reason for hiding this comment

dannykopping Dec 21, 2022

Choose a reason for hiding this comment

liguozhong Dec 26, 2022

Choose a reason for hiding this comment

dannykopping Dec 21, 2022

Choose a reason for hiding this comment

liguozhong Dec 26, 2022

Choose a reason for hiding this comment

liguozhong Dec 26, 2022 • edited Loading

Choose a reason for hiding this comment

adityacs Dec 25, 2022 • edited Loading

Choose a reason for hiding this comment

liguozhong Dec 26, 2022

Choose a reason for hiding this comment

adityacs Dec 27, 2022

Choose a reason for hiding this comment

liguozhong Dec 27, 2022

Choose a reason for hiding this comment

grafanabot commented Dec 26, 2022

owen-d left a comment • edited Loading

Choose a reason for hiding this comment

grafanabot commented Jan 4, 2023

owen-d left a comment

Choose a reason for hiding this comment

grafanabot commented Jan 4, 2023

SHaaD94 commented Feb 18, 2023 • edited Loading

liguozhong commented Dec 21, 2022 •

edited

Loading

liguozhong Dec 26, 2022 •

edited

Loading

adityacs Dec 25, 2022 •

edited

Loading

owen-d left a comment •

edited

Loading

SHaaD94 commented Feb 18, 2023 •

edited

Loading