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
Cache range filter on date field by default #7122
Cache range filter on date field by default #7122
Conversation
@martijnvg Could you review my PR? Thanks! :) |
} else { | ||
String value = convertToString(lowerTerm); | ||
cache = explicitCaching || !hasNowExpressionWithNoRounding(value); | ||
cache = (explicitCaching != null && explicitCaching) || !hasNowExpressionWithNoRounding(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martijnvg The more I look at this, the more I think there something wrong here.
I tried to fix an issue when you explicitly set _cache:false
. But in that case, even if you don't want to cache the filter, as soon as you don't use now
, it will be cached because of || !hasNowExpressionWithNoRounding(value)
.
That's what we have in the past, so it's not a regression here.
Though, I think I need to update the PR with that finding.
Will rebase on master as recent changes - support timezone
- created a git conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I missed this comment. You're right. If explicit caching is configured and set to true, we override it to false if now
without rounding is used. If no explicit caching is defined we should set cache to true if now
with no rounding is specified.
It should look something like this:
if (explicitCaching != null) {
if (explicitCaching) {
cache = hasNowExpressionWithNoRounding(value) == false;
} else {
cache = false;
}
} else {
cache = hasNowExpressionWithNoRounding(value) == false;
}
} | ||
|
||
|
||
private boolean computeCache(Boolean explicitCaching, boolean defaultValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method looks unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh! I forgot to remove it! :)
Left two comments, also the SimpleQueryTests#testRangeFilterNoCacheWithNow fails, because it assumes that a filter with now is being cached and that is no longer the case. Can you change that assertion as well? |
I don't understand. When I run test locally, I don't get any error for test |
LGTM, I find the code much more understandable than was before! |
A range filter on a date field with a numeric `from`/`to` value is **not** cached by default: DELETE /test PUT /test/t/1 { "date": "2014-01-01" } GET /_validate/query?explain { "query": { "filtered": { "filter": { "range": { "date": { "from": 0 } } } } } } Returns: "explanation": "ConstantScore(no_cache(date:[0 TO *]))" This patch fixes as well not caching `from`/`to` when using `now` value not rounded. Previously, a query like: GET /_validate/query?explain { "query": { "filtered": { "filter": { "range": { "date": { "from": "now" "to": "now/d+1" } } } } } } was cached. Also, this patch does not cache anymore `now` even if the user asked for caching it. As it won't be cached at all by definition. Added as well tests for all possible combinations. Closes elastic#7114.
A range filter on a date field with a numeric
from
/to
value is not cached by default:Returns:
This patch fixes as well not caching
from
/to
when usingnow
value not rounded.Previously, a query like:
was cached.
Also, this patch does not cache anymore
now
even if the user asked for caching it.As it won't be cached at all by definition.
Added as well tests for all possible combinations.
Closes #7114.