This repository was archived by the owner on Nov 7, 2025. It is now read-only.
Filter(s) 2nd approach
#927
Merged
Conversation
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
trzysiek
force-pushed
the
clover-filter-2nd-approach
branch
from
3edc8b0 to
b125483
Compare
trzysiek
force-pushed
the
clover-filter-2nd-approach
branch
from
5e3ddc0 to
d8b4f92
Compare
trzysiek
force-pushed
the
clover-filter-2nd-approach
branch
from
99e2438 to
fc17222
Compare
trzysiek
force-pushed
the
clover-filter-2nd-approach
branch
2 times, most recently
from
4e2c855 to
fe27c07
Compare
trzysiek
commented
Oct 30, 2024
Comment on lines
186
to
+195
| t1 := testName == "date_range aggregation(file:agg_req,nr:22)" // we use relative time | ||
| t2 := testName == "complex filters(file:agg_req,nr:18)" // almost, we differ in doc 0 counts | ||
| // to be deleted after pancakes | ||
| t3 := testName == "clients/kunkka/test_0, used to be broken before aggregations merge fix"+ | ||
| t2 := testName == "clients/kunkka/test_0, used to be broken before aggregations merge fix"+ | ||
| "Output more or less works, but is different and worse than what Elastic returns."+ | ||
| "If it starts failing, maybe that's a good thing(file:clients/kunkka,nr:0)" | ||
| // below test is replacing it | ||
| // testName == "it's the same input as in previous test, but with the original output from Elastic."+ | ||
| // "Skipped for now, as our response is different in 2 things: key_as_string date (probably not important) + we don't return 0's (e.g. doc_count: 0)."+ | ||
| // "If we need clients/kunkka/test_0, used to be broken before aggregations merge fix" | ||
| return t1 || t2 || t3 | ||
| return t1 || t2 |
Member
Author
There was a problem hiding this comment.
Seems like we're skipping more tests after this PR, but we actually skip one less (the one I remove here)
trzysiek
force-pushed
the
clover-filter-2nd-approach
branch
from
fe27c07 to
d08a273
Compare
Deploying quesma with
|
| Latest commit: |
aa4d576
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://db0dca0a.quesma.pages.dev |
| Branch Preview URL: | https://clover-filter-2nd-approach.quesma.pages.dev |
trzysiek
force-pushed
the
clover-filter-2nd-approach
branch
from
5453977 to
aed1d92
Compare
jakozaur
reviewed
Nov 1, 2024
Contributor
jakozaur
left a comment
jakozaur
left a comment
There was a problem hiding this comment.
Ok, that's good, we may wish to merge it anyway.
I'm still puzzled with this approach. Way better than previous one, but adding to top where might break total count as well as not work if it's inside. Instead, I would likely keep combinator logic, but just one filter per query each.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
So, this isn't a full solution.
Before we only knew how to handle
filter(s)when they're leaves (or almost) in the aggregation tree.Now we still can do that, but we also handle when they're the first aggregation layer (we just split them into multiple pancakes, with different
WHERE)I find it likely that we won't really need anything more, as those 2 cases exhaust both all our tests, and customer's case, so maybe it's hard to make a dashboard in Kibana with filter in the middle.
But anyway even if we want to support that, adding most top-level filter to where clause seems like a special case and would still be the way to go, so this PR should be a step forward.