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

Add time zone setting for relative date math in range filter/query #7113

Closed

Conversation

Projects
None yet
4 participants
@dadoonet
Copy link
Member

dadoonet commented Jul 31, 2014

Filters and Queries now supports time_zone parameter which defines which time zone should be applied to the query or filter to convert it to UTC time based value.

When applied on date fields the range filter and queries accept also a time_zone parameter.

The time_zone parameter will be applied to your input lower and upper bounds and will move them to UTC time based date:

{
    "constant_score": {
        "filter": {
            "range" : {
                "born" : {
                    "gte": "2012-01-01",
                    "lte": "now",
                    "time_zone": "+1:00"
                }
            }
        }
    }
}

{
    "range" : {
        "born" : {
            "gte": "2012-01-01",
            "lte": "now",
            "time_zone": "+1:00"
        }
    }
}

In the above examples, gte will be actually moved to 2011-12-31T23:00:00 UTC date.

NOTE: if you give a date with a timezone explicitly defined and use the time_zone parameter, time_zone will be
ignored. For example, setting from to 2012-01-01T00:00:00+01:00 with "time_zone":"+10:00" will still use +01:00 time zone.

Closes #3729.

Search: add time zone setting for relative date math in range filter/…
…query

Filters and Queries now supports `time_zone` parameter which defines which time zone should be applied to the query or filter to convert it to UTC time based value.

Closes #3729.

@dadoonet dadoonet self-assigned this Jul 31, 2014

@dadoonet dadoonet added the review label Jul 31, 2014

}

public long adjustToUTCTime(long value, DateTimeZone zone) {
// Small optimization: we don't do math when not needed :)

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 1, 2014

Contributor

Not sure it is needed? I would expect DateTimeZone.UTC.getOffset to already have this optimization and constantly return 0?

This comment has been minimized.

Copy link
@dadoonet

dadoonet Aug 1, 2014

Author Member

Agreed. Removed.

@@ -143,7 +151,7 @@ public Filter parse(QueryParseContext parseContext) throws IOException, QueryPar
throw new QueryParsingException(parseContext.index(), "[range] filter field [" + fieldName + "] is not a numeric type");
}
if (mapper instanceof DateFieldMapper) {
filter = ((DateFieldMapper) mapper).rangeFilter(parseContext, from, to, includeLower, includeUpper, parseContext, explicitlyCached);
filter = ((DateFieldMapper) mapper).rangeFilter(parseContext, from, to, includeLower, includeUpper, timeZone, parseContext, explicitlyCached);
} else {
filter = ((NumberFieldMapper) mapper).rangeFilter(parseContext, from, to, includeLower, includeUpper, parseContext);

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 1, 2014

Contributor

maybe add an exception here too that timezone can only be applied to dates?

This comment has been minimized.

Copy link
@dadoonet

dadoonet Aug 1, 2014

Author Member

Nice catch. I missed that one!

return DateTimeZone.forID(text);
}
}

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 1, 2014

Contributor

Let's do the same in DateHistogramFacetParser ?

This comment has been minimized.

Copy link
@dadoonet

dadoonet Aug 1, 2014

Author Member

Sure.

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 1, 2014

Instead of always moving the time after the date is parser, maybe we should rather configure the parser to use the provided time zone as a default, ie. when a timezone is not already provided in the date? This way, dates that are explicit about the timezone, eg.

  • "2012-04-21T18:25:43-05:00"
  • "2012-04-23T18:25:43.511Z"

would not be moved, while dates that don't specify a timezone, eg.

  • "2012-01-01"
    would be moved?

This seems to be possible by replacing parser.parseMillis(date); with parser.withZone(timeZone).parseMillis(date); (didn't try).

I think we should not try to move numbers of milliseconds since Epoch either, since Epoch is the same instant on all timezones.

@jpountz jpountz removed the review label Aug 1, 2014

@dadoonet

This comment has been minimized.

Copy link
Member Author

dadoonet commented Aug 1, 2014

@jpountz PR updated

@dadoonet dadoonet added the review label Aug 1, 2014

@@ -30,6 +30,29 @@ The `range` filter accepts the following parameters:
`lte`:: Less-than or equal to
`lt`:: Less-than

When applied on `date` fields the `range` filter accepts also a `time_zone` parameter.
The `time_zone` parameter will be applied to your input lower and upper bounds and will
move them to UTC time based date:

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 1, 2014

Contributor

I think we need to document that it is only applied for dates that don't make the timezone explicit?

This comment has been minimized.

Copy link
@dadoonet

dadoonet Aug 1, 2014

Author Member

They'll get a clean exception in that case :) But Yes I can add a note saying that they can't mix things.

DateTimeFormatter parser = dateTimeFormatter.parser();
if (timeZone != null) {
parser = parser.withZone(timeZone);
}

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 1, 2014

Contributor

given that you do it in a couple of methods, maybe this can be extracted to a private static utility method

@dadoonet

This comment has been minimized.

Copy link
Member Author

dadoonet commented Aug 1, 2014

@jpountz Updated :)

In the above example, `gte` will be actually moved to `2011-12-31T23:00:00` UTC date.

NOTE: you can not enter a date with a timezone explicitly defined with the `time_zone` parameter.
For example, setting `from` to `2012-01-01T00:00:00+01:00` will fail if you set any `time_zone`.

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 4, 2014

Contributor

I just tried it and didn't get any error, the time zone was just ignored (which is fine I think)

}

public Filter rangeFilter(QueryParseContext parseContext, Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, @Nullable QueryParseContext context, boolean explicitCaching) {
public Filter rangeFilter(QueryParseContext parseContext, Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, @Nullable DateTimeZone timeZone, @Nullable QueryParseContext context, boolean explicitCaching) {

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 4, 2014

Contributor

Can you quickly document the timeZone parameter, at least to mention that it is only applied when the String to parse doesn't have an explicit timezone

/**
* In case of date field, we can adjust the from/to fields using a timezone
*/
public RangeFilterBuilder timeZone(String preZone) {

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 4, 2014

Contributor

s/preZone/timeZone/

@@ -39,14 +41,26 @@ public DateMathParser(FormatDateTimeFormatter dateTimeFormatter, TimeUnit timeUn
}

public long parse(String text, long now) {
return parse(text, now, false);
return parse(text, now, false, DateTimeZone.UTC);

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 4, 2014

Contributor

Should you pass null here to make sure we are consistent with the previous behavior?

}

public long parseRoundCeil(String text, long now) {
return parse(text, now, true);
return parse(text, now, true, DateTimeZone.UTC);

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 4, 2014

Contributor

null?

}

public long parse(String text, long now, boolean roundCeil) {
return parse(text, now, roundCeil, DateTimeZone.UTC);

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 4, 2014

Contributor

null?

try {
return dateTimeFormatter.parser().parseMillis(value);
DateTimeFormatter parser = getDateTimeFormatterParser(dateTimeFormatter, timeZone);
return parser.parseMillis(value);
} catch (RuntimeException e) {

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 4, 2014

Contributor

In this method, can you add a comment that you don't apply the timeZone to numbers of milliseconds since Epoch on purpose? Otherwise I think someone quickly looking at the code might think it is a bug.

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 4, 2014

@dadoonet Just played with the pull request, seems to work fine. I left a couple of minor comments but other than that it looks good to me!

@jpountz jpountz removed the review label Aug 4, 2014

@dadoonet

This comment has been minimized.

Copy link
Member Author

dadoonet commented Aug 4, 2014

@jpountz thanks for the comments. Just updated my branch.

I'll rebase on master and run the full tests again.

Do you think it should go in other branch than master and 1.4?

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 4, 2014

+1 to push, this looks great!

Do you think it should go in other branch than master and 1.4?

master and 1.4 sound good to me. This is a new feature, so it doesn't really make sense to push it to older branches.

@dadoonet

This comment has been minimized.

Copy link
Member Author

dadoonet commented Aug 4, 2014

I guess I need to update doc with a "coming in 1.4" :)

@dadoonet

This comment has been minimized.

Copy link
Member Author

dadoonet commented Aug 4, 2014

pushed in master (873a45e) and 1.x (2c0db1f)

@dadoonet dadoonet closed this Aug 4, 2014

@dadoonet dadoonet deleted the dadoonet:pr/3729-timezone-rangefilter branch Aug 4, 2014

@clintongormley clintongormley changed the title Search: add time zone setting for relative date math in range filter/query Search: Add time zone setting for relative date math in range filter/query Sep 8, 2014

@clintongormley clintongormley changed the title Search: Add time zone setting for relative date math in range filter/query Query DSL: Add time zone setting for relative date math in range filter/query Sep 8, 2014

@mewwts

This comment has been minimized.

Copy link

mewwts commented Feb 23, 2015

Hi, I've opened #9814 because I can't seem to get this to work with rounding in date math. Is this expected behaviour?

@clintongormley clintongormley changed the title Query DSL: Add time zone setting for relative date math in range filter/query Add time zone setting for relative date math in range filter/query 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.