Skip to content
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.

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

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

Open
alanprot opened this issue Apr 29, 2023 · 2 comments

Comments

@alanprot
Copy link
Contributor

alanprot commented Apr 29, 2023

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.

@MichaHoffmann
Copy link
Contributor

MichaHoffmann commented May 18, 2023

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
Copy link
Contributor

MichaHoffmann commented May 18, 2023

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))))`,
		},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants