Use date format in `date_range` mapping before fallback to default #29310

liketic · 2018-03-30T05:30:10Z

If the date format is not forced in query, use the format in mapping before fallback to the default format.

elasticmachine · 2018-03-30T05:30:12Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2018-03-30T05:30:12Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2018-04-03T15:29:46Z

Pinging @elastic/es-search-aggs

cbuescher

Hi @liketic, yet another great PR, thanks for picking this up. I think the fix in RangeFieldMapper is good, however I'd like the test to be less generic and randomized. In its current form it seems to test all kinds of ranges (not only date), many versions etc. while I would be convinced more that the fix works if it would stick closer to the scenario described in #29282. I'd remove a lot of the randomization and make this test more specialized about the scenario described there I think.

liketic · 2018-04-06T13:15:10Z

Thanks @cbuescher ! I updated the PR. Please review again. 👍

cbuescher

Hi @liketic, thanks for reworking the test. I'm not sure I'm convinced about what the test does yet, I think currently it might not do the right thing (at the moment is passes with or without your change in the RangeFieldMapper).
I hope the comment I left makes sense, happy to clear things up if you have questions.

cbuescher · 2018-04-06T16:34:51Z

server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java

+        assertEquals(
+            RangeType.DATE.rangeQuery(FIELDNAME, true, from, to, true,
+                true, relation, null, defaultParser, context),
+            fieldType.rangeQuery(from, to, true, true, relation, null, null, context));


I'm not sure what is tested here, can you explain?
As far as I can see, fieldType.rangeQuery only redirects to DATE#rangeQuery, so other than checking if the parser gets picked up, this test is not doing much in my opinion, but I might miss something.
I'd prefer it it would check that the from/to values are parsed correctly using the parser supplied by fieldType.setDateTimeFormatter. For that I think the from/to values should be String representations of dates that DEFAULT_DATE_TIME_FORMATTER would reject otherwise. I'd like to see a test that checks that using the DEFAULT_DATE_TIME_FORMATTER we get an error (expectThrows) and with the right parser the query contains the expected min/max values. Accessing the min/max values of the Lucene query might be a bit tricky on first inspection of the code, but I think most shape relations should produce LongRange queries which Query can be cast to to use the getters.

cbuescher · 2018-05-03T11:31:28Z

@liketic checking in to see if we can get this PR in soon. The fix is fine I think, only the test needs some reworking as I mentioned in the comments. Let me know what you think and if you have time to get back to this in the next week or so.

liketic · 2018-05-03T12:00:34Z

@cbuescher My apologies for the delay. Thanks for your comments. I'll fix it before next week.

cbuescher · 2018-05-03T12:20:49Z

@liketic no problem at all, I didn't want to set a date either. Just checking if you need anything, please take your time.

…sues/29282

cbuescher

@liketic thanks a lot, this looks good to me. I will kick of a run of CI testing. There seems to be a new requirement to add changelog information to PRs that I'm not completely up to date with myself but I will come back with some info on what we need here (if anything).

cbuescher · 2018-05-07T11:56:09Z

@elasticmachine test this please

cbuescher · 2018-05-07T13:21:14Z

@liketic we seem to require additions to a changlelog file now for bugfixes and features. Could you add a line mentioning this PRs title and number similar to e.g. https://github.com/elastic/elasticsearch/pull/29641/files#diff-297418c07078d2b31d8e94a7d34b5255 to the docs/CHANGELOG.asciidoc. I think this fix should also be backported to the next 6.x branches. At the moment I believe this means putting the same entry in two places (under 7.0 and 6.4). Thanks a lot.

cbuescher · 2018-05-07T18:33:43Z

@liketic when you add the changelog, could you also merge in master once more, CI run didn't pass but its not related to this change I think.

cbuescher · 2018-05-08T13:08:36Z

@liketic I found out how to push to user PRs and added the changlog entry and merged in master, hope you don't mind.
@elasticmachine test this please

cbuescher · 2018-05-08T14:02:52Z

@elasticmachine test this please

cbuescher · 2018-05-08T17:39:42Z

@elasticmachine test this please

cbuescher · 2018-05-09T07:42:10Z

@liketic finally a green CI run. I merged this to master and will also port it to the 6.4 branch.

…29310) If the date format is not forced in query, use the format in mapping before fallback to the default format. Closes #29282

…or-you * elastic/master: (22 commits) Docs: Test examples that recreate lang analyzers (elastic#29535) BulkProcessor to retry based on status code (elastic#29329) Add GET Repository High Level REST API (elastic#30362) add a comment explaining the need for RetryOnReplicaException on missing mappings Add `coordinating_only` node selector (elastic#30313) Stop forking groovyc (elastic#30471) Avoid setting connection request timeout (elastic#30384) Use date format in `date_range` mapping before fallback to default (elastic#29310) Watcher: Increase HttpClient parallel sent requests (elastic#30130) Mute ML upgrade test (elastic#30458) Stop forking javac (elastic#30462) Client: Deprecate many argument performRequest (elastic#30315) Docs: Use task_id in examples of tasks (elastic#30436) Security: Rename IndexLifecycleManager to SecurityIndexManager (elastic#30442) [Docs] Fix typo in cardinality-aggregation.asciidoc (elastic#30434) Avoid NPE in `more_like_this` when field has zero tokens (elastic#30365) Build: Switch to building javadoc with html5 (elastic#30440) Add a quick tour of the project to CONTRIBUTING (elastic#30187) Reindex: Use request flavored methods (elastic#30317) Silence SplitIndexIT.testSplitIndexPrimaryTerm test failure. (elastic#30432) ...

* master: Upgrade to Lucene-7.4-snapshot-6705632810 (#30519) add version compatibility from 6.4.0 after backport, see #30319 (#30390) Security: Simplify security index listeners (#30466) Add proper longitude validation in geo_polygon_query (#30497) Remove Discovery.AckListener.onTimeout() (#30514) Build: move generated-resources to build (#30366) Reindex: Fold "with all deps" project into reindex (#30154) Isolate REST client single host tests (#30504) Solve Gradle deprecation warnings around shadowJar (#30483) SAML: Process only signed data (#30420) Remove BWC repository test (#30500) Build: Remove xpack specific run task (#30487) AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests LLClient: Add setJsonEntity (#30447) Expose CommonStatsFlags directly in IndicesStatsRequest. (#30163) Silence IndexUpgradeIT test failures. (#30430) Bump Gradle heap to 1792m (#30484) [docs] add warning for read-write indices in force merge documentation (#28869) Avoid deadlocks in cache (#30461) Test: remove hardcoded list of unconfigured ciphers (#30367) mute SplitIndexIT due to #30416 Docs: Test examples that recreate lang analyzers (#29535) BulkProcessor to retry based on status code (#29329) Add GET Repository High Level REST API (#30362) add a comment explaining the need for RetryOnReplicaException on missing mappings Add `coordinating_only` node selector (#30313) Stop forking groovyc (#30471) Avoid setting connection request timeout (#30384) Use date format in `date_range` mapping before fallback to default (#29310) Watcher: Increase HttpClient parallel sent requests (#30130) # Conflicts: # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java

* 6.x: Upgrade to Lucene-7.4-snapshot-6705632810 (#30519) Remove Discovery.AckListener.onTimeout() (#30514) Build: move generated-resources to build (#30366) Reindex: Fold "with all deps" project into reindex (#30154) Isolate REST client single host tests (#30504) Remove BWC repository test (#30500) Build: Remove xpack specific run task (#30487) AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests LLClient: Add setJsonEntity (#30447) [docs] add warning for read-write indices in force merge documentation (#28869) Avoid deadlocks in cache (#30461) BulkProcessor to retry based on status code (#29329) Avoid setting connection request timeout (#30384) Test: remove hardcoded list of unconfigured ciphers (#30367) Add GET Repository High Level REST API (#30362) mute SplitIndexIT due to #30416 Docs: Test examples that recreate lang analyzers (#29535) add a comment explaining the need for RetryOnReplicaException on missing mappings Pass the task to broadcast actions (#29672) Stop forking groovyc (#30471) Add `coordinating_only` node selector (#30313) Fix accidental error in changelog Use date format in `date_range` mapping before fallback to default (#29310) Watcher: Increase HttpClient parallel sent requests (#30130) [Security][Tests] Azeri(Turkish) locale tripps opensaml dependency

liketic · 2018-05-12T08:03:46Z

Thanks @cbuescher !

Use date format in mapping before fallback to default if not force fo…

27c84c1

…rmat (elastic#29282)

cbuescher self-assigned this Apr 3, 2018

cbuescher self-requested a review April 3, 2018 15:29

cbuescher added the :Search/Search Search-related issues that do not fall into other categories label Apr 3, 2018

cbuescher added the review label Apr 3, 2018

cbuescher requested changes Apr 4, 2018

View reviewed changes

Fix test

9e0c04e

cbuescher requested changes Apr 6, 2018

View reviewed changes

cbuescher mentioned this pull request May 2, 2018

RangeFieldType.rangeQuery now defaults to mappings format(parser) #30252

Closed

liketic added 2 commits May 6, 2018 09:25

Merge branch 'master' of github.com:elastic/elasticsearch into fix-is…

f419a34

…sues/29282

Fix test

3141e88

cbuescher added the >bug label May 7, 2018

cbuescher approved these changes May 7, 2018

View reviewed changes

cbuescher added v7.0.0 v6.4.0 labels May 7, 2018

cbuescher changed the title ~~Use date format in mapping before fallback to default~~ Use date format in date_range mapping before fallback to default May 7, 2018

Christoph Büscher added 2 commits May 8, 2018 14:38

Merge branch 'master' into liketic/fix-issues/29282

dc45537

Update changelog

42c0ff8

Merge branch 'master' into fix-issues/29282

2efc676

Merge branch 'master' into fix-issues/29282

66f34ac

cbuescher merged commit 0c6789b into elastic:master May 9, 2018

cbuescher pushed a commit that referenced this pull request May 9, 2018

Use date format in date_range mapping before fallback to default (#…

5ec9041

…29310) If the date format is not forced in query, use the format in mapping before fallback to the default format. Closes #29282

liketic deleted the fix-issues/29282 branch May 12, 2018 08:03

cbuescher mentioned this pull request Jun 18, 2018

Unable to open index in Discover and Visualize when date format is epoch_millis elastic/kibana#19858

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use date format in `date_range` mapping before fallback to default #29310

Use date format in `date_range` mapping before fallback to default #29310

liketic commented Mar 30, 2018 •

edited

Loading

elasticmachine commented Mar 30, 2018

elasticmachine commented Mar 30, 2018

elasticmachine commented Apr 3, 2018

cbuescher left a comment

liketic commented Apr 6, 2018

cbuescher left a comment

cbuescher Apr 6, 2018

cbuescher commented May 3, 2018

liketic commented May 3, 2018

cbuescher commented May 3, 2018

cbuescher left a comment

cbuescher commented May 7, 2018

cbuescher commented May 7, 2018

cbuescher commented May 7, 2018

cbuescher commented May 8, 2018

cbuescher commented May 8, 2018

cbuescher commented May 8, 2018

cbuescher commented May 9, 2018

liketic commented May 12, 2018

Use date format in date_range mapping before fallback to default #29310

Use date format in date_range mapping before fallback to default #29310

Conversation

liketic commented Mar 30, 2018 • edited Loading

elasticmachine commented Mar 30, 2018

elasticmachine commented Mar 30, 2018

elasticmachine commented Apr 3, 2018

cbuescher left a comment

Choose a reason for hiding this comment

liketic commented Apr 6, 2018

cbuescher left a comment

Choose a reason for hiding this comment

cbuescher Apr 6, 2018

Choose a reason for hiding this comment

cbuescher commented May 3, 2018

liketic commented May 3, 2018

cbuescher commented May 3, 2018

cbuescher left a comment

Choose a reason for hiding this comment

cbuescher commented May 7, 2018

cbuescher commented May 7, 2018

cbuescher commented May 7, 2018

cbuescher commented May 8, 2018

cbuescher commented May 8, 2018

cbuescher commented May 8, 2018

cbuescher commented May 9, 2018

liketic commented May 12, 2018

Use date format in `date_range` mapping before fallback to default #29310

Use date format in `date_range` mapping before fallback to default #29310

liketic commented Mar 30, 2018 •

edited

Loading