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

Cache range filter on date field by default #7122

Merged
merged 1 commit into from Aug 12, 2014

Conversation

Projects
None yet
3 participants
@dadoonet
Copy link
Member

dadoonet commented Aug 1, 2014

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 #7114.

@dadoonet dadoonet added review labels Aug 1, 2014

@dadoonet dadoonet self-assigned this Aug 1, 2014

@dadoonet

This comment has been minimized.

Copy link
Member Author

dadoonet commented Aug 1, 2014

@martijnvg Could you review my PR? Thanks! :)

@dadoonet

View changes

src/main/java/org/elasticsearch/index/mapper/core/DateFieldMapper.java Outdated
} else {
String value = convertToString(lowerTerm);
cache = explicitCaching || !hasNowExpressionWithNoRounding(value);
cache = (explicitCaching != null && explicitCaching) || !hasNowExpressionWithNoRounding(value);

This comment has been minimized.

Copy link
@dadoonet

dadoonet Aug 6, 2014

Author Member

@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.

This comment has been minimized.

Copy link
@martijnvg

martijnvg Aug 8, 2014

Member

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;
}
@martijnvg

View changes

src/main/java/org/elasticsearch/index/mapper/core/DateFieldMapper.java Outdated
}


private boolean computeCache(Boolean explicitCaching, boolean defaultValue) {

This comment has been minimized.

Copy link
@martijnvg

martijnvg Aug 12, 2014

Member

this method looks unused?

This comment has been minimized.

Copy link
@dadoonet

dadoonet Aug 12, 2014

Author Member

Argh! I forgot to remove it! :)

@martijnvg

View changes

src/main/java/org/elasticsearch/index/mapper/core/DateFieldMapper.java Outdated
upperVal = parseToMilliseconds(value, context, includeUpper, timeZone);
}
}

if (explicitCaching != null) {
if (explicitCaching) {
cache = nowExpressionWithNoRounding == false;

This comment has been minimized.

Copy link
@martijnvg

martijnvg Aug 12, 2014

Member

maybe just: cache = nowExpressionWithNoRounding; instead?

This comment has been minimized.

Copy link
@martijnvg

martijnvg Aug 12, 2014

Member

wait that doesn't work should be: cache = !nowExpressionWithNoRounding; which is the same as you have now but less verbose, I don't like negation at all, since it confuses me easily, but then we need to change the variable name to nowExpressionWithRounding and change the helper method. What do you think about that?

This comment has been minimized.

Copy link
@dadoonet

dadoonet Aug 12, 2014

Author Member

Actually it's a variable naming concern. Using nowExpressionWithRounding will make me think that the user has entered now/d. But, nowExpressionWithRounding will be true as well if user entered 2012-01-01 which is not a now date.

May be using notCacheable makes it more clear?

Still, we will use cache = !notCacheable.

Or if we use cacheable or nowExpressionWithRounding, we still have that negation but not at the same place:

        boolean cacheable = true;
        Long lowerVal = null;
        Long upperVal = null;
        if (lowerTerm != null) {
            if (lowerTerm instanceof Number) {
                lowerVal = ((Number) lowerTerm).longValue();
            } else {
                String value = convertToString(lowerTerm);
                cacheable = !hasNowExpressionWithNoRounding(value);
                lowerVal = parseToMilliseconds(value, context, false, timeZone);
            }
        }
        if (upperTerm != null) {
            if (upperTerm instanceof Number) {
                upperVal = ((Number) upperTerm).longValue();
            } else {
                String value = convertToString(upperTerm);
                cacheable = cacheable && !hasNowExpressionWithNoRounding(value);
                upperVal = parseToMilliseconds(value, context, includeUpper, timeZone);
            }
        }

        if (explicitCaching != null) {
            if (explicitCaching) {
                cache = cacheable;
            } else {
                cache = false;
            }
        } else {
            cache = cacheable;
        }

WDYT?

@martijnvg

This comment has been minimized.

Copy link
Member

martijnvg commented Aug 12, 2014

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?

@dadoonet

This comment has been minimized.

Copy link
Member Author

dadoonet commented Aug 12, 2014

I don't understand. When I run test locally, I don't get any error for test org.elasticsearch.search.query.SimpleQueryTests#testRangeFilterNoCacheWithNow. Could you paste your seed?

@martijnvg

This comment has been minimized.

Copy link
Member

martijnvg commented Aug 12, 2014

LGTM, I find the code much more understandable than was before!

Query DSL: Cache range filter on date field by default
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 #7114.

@dadoonet dadoonet merged commit 9e68687 into elastic:master Aug 12, 2014

@dadoonet dadoonet deleted the dadoonet:issue/7114-cache-date-range-ms branch Aug 12, 2014

@clintongormley clintongormley added the >bug label Sep 8, 2014

@clintongormley clintongormley changed the title Query DSL: Cache range filter on date field by default Cache range filter on date field by default Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.