From d7a43b0f065e7c3b50b89eeb98ea67012574776d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Wed, 7 Oct 2020 13:31:52 +0200 Subject: [PATCH 1/2] Handle range query edge case Currently when searching with an empty string as lower bound for a range query on text-based fields we return all documents when 'gte' is used (including the lower bound) but no documents when 'gt' is used. This might seem counterintuitive since every value should be greate than the empty string. This PR fixed this special edge case by implicitely setting the "lower" include flag in this case before constructing the TermRangeQuery. Closes #63386 --- .../search/simple/SimpleSearchIT.java | 40 +++++++++++++++++++ .../index/mapper/StringFieldType.java | 6 ++- .../index/mapper/TextFieldTypeTests.java | 7 ++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/simple/SimpleSearchIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/simple/SimpleSearchIT.java index 6a163543e3073..0f7b0d5448ae6 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/simple/SimpleSearchIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/simple/SimpleSearchIT.java @@ -225,6 +225,46 @@ public void testSimpleDateRange() throws Exception { assertHitCount(searchResponse, 2L); } + public void testRangeQueryKeyword() throws Exception { + createIndex("test"); + + client().admin().indices().preparePutMapping("test").setSource("field", "type=keyword").get(); + + client().prepareIndex("test").setId("1").setSource("field", "A").get(); + client().prepareIndex("test").setId("2").setSource("field", "B").get(); + client().prepareIndex("test").setId("3").setSource("field", "C").get(); + ensureGreen(); + refresh(); + + SearchResponse searchResponse = client().prepareSearch("test").setQuery(QueryBuilders.rangeQuery("field").gte("A").lte("B")).get(); + assertNoFailures(searchResponse); + assertHitCount(searchResponse, 2L); + + searchResponse = client().prepareSearch("test").setQuery(QueryBuilders.rangeQuery("field").gt("A").lte("B")).get(); + assertNoFailures(searchResponse); + assertHitCount(searchResponse, 1L); + + searchResponse = client().prepareSearch("test").setQuery(QueryBuilders.rangeQuery("field").gte("A").lt("B")).get(); + assertNoFailures(searchResponse); + assertHitCount(searchResponse, 1L); + + searchResponse = client().prepareSearch("test").setQuery(QueryBuilders.rangeQuery("field").gte(null).lt("C")).get(); + assertNoFailures(searchResponse); + assertHitCount(searchResponse, 2L); + + searchResponse = client().prepareSearch("test").setQuery(QueryBuilders.rangeQuery("field").gte("B").lt(null)).get(); + assertNoFailures(searchResponse); + assertHitCount(searchResponse, 2L); + + searchResponse = client().prepareSearch("test").setQuery(QueryBuilders.rangeQuery("field").gt(null).lt(null)).get(); + assertNoFailures(searchResponse); + assertHitCount(searchResponse, 3L); + + searchResponse = client().prepareSearch("test").setQuery(QueryBuilders.rangeQuery("field").gt("").lt(null)).get(); + assertNoFailures(searchResponse); + assertHitCount(searchResponse, 3L); + } + public void testSimpleTerminateAfterCount() throws Exception { prepareCreate("test").setSettings(Settings.builder().put(SETTING_NUMBER_OF_SHARDS, 1).put(SETTING_NUMBER_OF_REPLICAS, 0)).get(); ensureGreen(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/StringFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/StringFieldType.java index 11da7ad7c7fc8..fa938636570b9 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/StringFieldType.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/StringFieldType.java @@ -170,8 +170,12 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower ALLOW_EXPENSIVE_QUERIES.getKey() + "' is set to false."); } failIfNotIndexed(); + BytesRef indexedLower = lowerTerm == null ? null : indexedValueForSearch(lowerTerm); + if (indexedLower != null && indexedLower.length == 0) { + includeLower = true; + } return new TermRangeQuery(name(), - lowerTerm == null ? null : indexedValueForSearch(lowerTerm), + indexedLower, upperTerm == null ? null : indexedValueForSearch(upperTerm), includeLower, includeUpper); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/TextFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/TextFieldTypeTests.java index 4978962ee4961..9ae088483680d 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/TextFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/TextFieldTypeTests.java @@ -95,6 +95,13 @@ public void testRangeQuery() { assertEquals(new TermRangeQuery("field", BytesRefs.toBytesRef("foo"), BytesRefs.toBytesRef("bar"), true, false), ft.rangeQuery("foo", "bar", true, false, null, null, null, MOCK_QSC)); + // empty string lower bound should always result query with lower-include flag being set + assertEquals(new TermRangeQuery("field", BytesRefs.toBytesRef(""), null, true, true), + ft.rangeQuery("", null, true, true, null, null, null, MOCK_QSC)); + + assertEquals(new TermRangeQuery("field", BytesRefs.toBytesRef(""), null, true, true), + ft.rangeQuery("", null, false, true, null, null, null, MOCK_QSC)); + ElasticsearchException ee = expectThrows(ElasticsearchException.class, () -> ft.rangeQuery("foo", "bar", true, false, null, null, null, MOCK_QSC_DISALLOW_EXPENSIVE)); assertEquals("[range] queries on [text] or [keyword] fields cannot be executed when " + From 3623b25c55f62a68065c3bbf58e69cb7624f1883 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 19 Oct 2020 14:31:15 +0200 Subject: [PATCH 2/2] only keep tests --- .../org/elasticsearch/search/simple/SimpleSearchIT.java | 9 +++++++-- .../org/elasticsearch/index/mapper/StringFieldType.java | 6 +----- .../elasticsearch/index/mapper/TextFieldTypeTests.java | 7 ------- 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/simple/SimpleSearchIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/simple/SimpleSearchIT.java index 0f7b0d5448ae6..2ef7a961fde68 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/simple/SimpleSearchIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/simple/SimpleSearchIT.java @@ -230,6 +230,7 @@ public void testRangeQueryKeyword() throws Exception { client().admin().indices().preparePutMapping("test").setSource("field", "type=keyword").get(); + client().prepareIndex("test").setId("0").setSource("field", "").get(); client().prepareIndex("test").setId("1").setSource("field", "A").get(); client().prepareIndex("test").setId("2").setSource("field", "B").get(); client().prepareIndex("test").setId("3").setSource("field", "C").get(); @@ -250,7 +251,7 @@ public void testRangeQueryKeyword() throws Exception { searchResponse = client().prepareSearch("test").setQuery(QueryBuilders.rangeQuery("field").gte(null).lt("C")).get(); assertNoFailures(searchResponse); - assertHitCount(searchResponse, 2L); + assertHitCount(searchResponse, 3L); searchResponse = client().prepareSearch("test").setQuery(QueryBuilders.rangeQuery("field").gte("B").lt(null)).get(); assertNoFailures(searchResponse); @@ -258,7 +259,11 @@ public void testRangeQueryKeyword() throws Exception { searchResponse = client().prepareSearch("test").setQuery(QueryBuilders.rangeQuery("field").gt(null).lt(null)).get(); assertNoFailures(searchResponse); - assertHitCount(searchResponse, 3L); + assertHitCount(searchResponse, 4L); + + searchResponse = client().prepareSearch("test").setQuery(QueryBuilders.rangeQuery("field").gte("").lt(null)).get(); + assertNoFailures(searchResponse); + assertHitCount(searchResponse, 4L); searchResponse = client().prepareSearch("test").setQuery(QueryBuilders.rangeQuery("field").gt("").lt(null)).get(); assertNoFailures(searchResponse); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/StringFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/StringFieldType.java index fa938636570b9..11da7ad7c7fc8 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/StringFieldType.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/StringFieldType.java @@ -170,12 +170,8 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower ALLOW_EXPENSIVE_QUERIES.getKey() + "' is set to false."); } failIfNotIndexed(); - BytesRef indexedLower = lowerTerm == null ? null : indexedValueForSearch(lowerTerm); - if (indexedLower != null && indexedLower.length == 0) { - includeLower = true; - } return new TermRangeQuery(name(), - indexedLower, + lowerTerm == null ? null : indexedValueForSearch(lowerTerm), upperTerm == null ? null : indexedValueForSearch(upperTerm), includeLower, includeUpper); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/TextFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/TextFieldTypeTests.java index 9ae088483680d..4978962ee4961 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/TextFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/TextFieldTypeTests.java @@ -95,13 +95,6 @@ public void testRangeQuery() { assertEquals(new TermRangeQuery("field", BytesRefs.toBytesRef("foo"), BytesRefs.toBytesRef("bar"), true, false), ft.rangeQuery("foo", "bar", true, false, null, null, null, MOCK_QSC)); - // empty string lower bound should always result query with lower-include flag being set - assertEquals(new TermRangeQuery("field", BytesRefs.toBytesRef(""), null, true, true), - ft.rangeQuery("", null, true, true, null, null, null, MOCK_QSC)); - - assertEquals(new TermRangeQuery("field", BytesRefs.toBytesRef(""), null, true, true), - ft.rangeQuery("", null, false, true, null, null, null, MOCK_QSC)); - ElasticsearchException ee = expectThrows(ElasticsearchException.class, () -> ft.rangeQuery("foo", "bar", true, false, null, null, null, MOCK_QSC_DISALLOW_EXPENSIVE)); assertEquals("[range] queries on [text] or [keyword] fields cannot be executed when " +