promql-engine icon indicating copy to clipboard operation
promql-engine copied to clipboard

`topK/bottomK` edge behavior mismatch between thanos engine and the native engine

Open alanprot opened this issue 2 years ago • 2 comments

The topK and bottomK operators return errors on the native engine when the first argument (k) is empty on all cases and in the Thanos engine, we only return an error if the second argument is not empty.

Ex: the following expression returns error on the native engine but not in the new engine:

bottomk(scalar(do_not_exist), do_not_exist)`

Adding a simple check to return an error always if the arg is nil does not work as it breaks some distributed queries tests where some shards can be empty.

alanprot avatar Apr 29 '23 00:04 alanprot

I wonder: is topk/bottomk even distributable? bottomk(X, Y) if X resides on one engine and Y resides on another seems broken, right? We probably only should distribute if the k is a *parser.NumberLiteral? In that case we can just add the check, right?

MichaHoffmann avatar May 18 '23 14:05 MichaHoffmann

Does this testcase make sense?

		{
			name:     "topk non-distributable",
			expr:     `topk(scalar(X), Y)`,
			expected: `topk(scalar(X), dedup(remote(Y), remote(Y)))`,
		},
		{
			name:     "topk distributable",
			expr:     `topk(1, Y)`,
			expected: `topk(1, dedup(remote(topk by (region) (1, Y)), remote(topk by (region) (1, Y))))`,
		},

MichaHoffmann avatar May 18 '23 18:05 MichaHoffmann