Allow docvalues-only search on number types #82409

ywelsch · 2022-01-11T11:03:14Z

Allows searching on number field types (long, short, int, float, double, byte, scaled_float, half_float) when those fields are not indexed (index: false) but just doc values are enabled.

This enables searches on archive data, which has access to doc values but not index structures. When combined with searchable snapshots, it allows downloading only data for a given (doc value) field to quickly filter down to a select set of documents.

Note to reviewers:

I have split isSearchable into two separate methods isIndexed and isSearchable on MappedFieldType. The former one is about whether actual indexing data structures have been used (postings or points), and the latter one on whether you can run queries on the given field (e.g. used by field caps). For number field types, queries are now allowed whenever points are available or when doc values are available (i.e. searchability is expanded).

Relates #81210 and #52728

…-search

elasticmachine · 2022-01-12T07:24:49Z

Pinging @elastic/es-search (Team:Search)

romseygeek

I had a first pass through, looks good to me for the most part. We should think about allowing an escape hatch via expensive query checks so that users can ensure they don't accidentally start running very slow queries.

romseygeek · 2022-01-12T10:50:02Z

server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java

        @Override
        public Query termQuery(Object value, SearchExecutionContext context) {
-            failIfNotIndexed();
-            return type.termQuery(name(), value);
+            failIfNotIndexedNorDocValues();


Do you think we should have an expensive query check here as well?

Works for me. Added check in bae5818

romseygeek · 2022-01-12T10:50:41Z

server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java

@@ -446,6 +453,13 @@ protected final void failIfNotIndexed() {
        }
    }

+    protected final void failIfNotIndexedNorDocValues() {


Maybe failIfNotSearchable?

I've renamed this (because of expensive queries check), but prefer the more concrete name.

Fair enough, and I like how the two checks are combined, very neat.

…-search

romseygeek

LGTM

javanna

I left some comments, the changes look great to me.

Relates to #52728 .

Do you plan on adding documentation too?

One thing that I would like to discuss is how we treat these doc_value queries as expensive ( I left a comment on that) and whether it's time to work on #48058 too to complete the picture. It seems like the allow expensive queries flag is no longer enough.

Thanks for working on this!

javanna · 2022-01-12T13:42:47Z

rest-api-spec/build.gradle

@@ -218,6 +218,8 @@ tasks.named("yamlRestTestV7CompatTransform").configure { task ->
  // sync_id is no longer available in SegmentInfos.userData // "indices.flush/10_basic/Index synced flush rest test"
  task.replaceIsTrue("indices.testing.shards.0.0.commit.user_data.sync_id", "indices.testing.shards.0.0.commit.user_data")

+  // we can now search using doc values only
+  task.replaceValueInMatch("fields.object\\.nested1.long.searchable", true)


can you remind me what this does?

This task runs the yaml test suite from ES 7.x against ES 8. By supporting searches on doc-values-only fields, we now have a "breaking" change as the test from 7.x returns different results, namely this field now appears as "searchable" in 8. This is ok though.

I see, thanks for explaining.

x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/v2/RollupShardIndexer.java

javanna · 2022-01-12T15:51:30Z

server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java

@@ -446,6 +456,22 @@ protected final void failIfNotIndexed() {
        }
    }

+    protected final void failIfNotIndexedNorDocValuesFallback(SearchExecutionContext context) {


one thing throws me off a bit here: the fact that a field is searchable or not effectively depends on whether expensive queries are allowed if only doc_values are present. For doc_value only fields though field_caps will always return true, yet may return an error when the field is searched?

On the other hand this is how expensive queries worked until now, even if the field is searchable they may throw error, but I am not sure we ever did this for basic queries like a term query. I believe this could be the first time that a field is effectively not searchable if expensive queries are disallowed, because no query can be performed against it.

The same applies to term/range/etc. queries on runtime fields while those are searchable as well, so this isn't something new. I also think that that's ok.

true, how could I forget about runtime fields? :) I also don't see field_caps output being based on whether expensive queries are allowed or not, though this is something to keep in mind as we introduce more expensive queries/fields.

javanna · 2022-01-12T16:03:38Z

server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java

+    /**
+     * Returns true if the field is indexed.
+     */
+    public final boolean isIndexed() {


Thanks, the distinction between searchable and indexed is something that we removed a while ago, if I remember correctly, but we ended up using isSearchable in many places where we really meant isIndexed instead and I found it confusing (even if the two meant the same until now). I find it much clearer now with this distinction, and I double checked that you fixed all the places that need to call isIndexed. Great!

javanna · 2022-01-12T16:08:59Z

server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java

@@ -266,9 +266,13 @@ public Float parse(XContentParser parser, boolean coerce) throws IOException {
            }

            @Override
-            public Query termQuery(String field, Object value) {
+            public Query termQuery(String field, Object value, boolean forceDocValues) {


nit: I am not entirely happy with the naming of the new parameter. I think forceDocValues fits better for range queries where we use IndexOrDocValuesQuery, but here it's more about either one or the other. I also struggle slightly with always providing isIndexed == false to it. Shall we rather call it isIndexed for termQuery ? I find that that would express better the fact that the index has the precedence if present.

ok, fixed in 4b273fd

javanna · 2022-01-12T16:11:20Z

server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java

@@ -289,7 +293,8 @@ public Query rangeQuery(
                boolean includeLower,
                boolean includeUpper,
                boolean hasDocValues,
-                SearchExecutionContext context
+                SearchExecutionContext context,
+                boolean forceDocValues


nit: I can see why the additional argument is called forceDocValues here, because we explicitly want to query only doc_values, but here too maybe isIndexed would also express that if the field is indexed we query that or doc_values, while if it's not indexed we can only query doc_values.

ok, fixed in 4b273fd

…-search

ywelsch · 2022-01-13T08:52:11Z

Do you plan on adding documentation too?

Not as part of this PR yet. For the core feature, I don't think this needs extra documentation. In the context of archive data (#81210), I want to add some docs once these pieces are integrated (i.e. mappings are automatically transferred over).

One thing that I would like to discuss is how we treat these doc_value queries as expensive ( I left a comment on that) and whether it's time to work on #48058 too to complete the picture. It seems like the allow expensive queries flag is no longer enough.

As noted on the comment, this PR keeps things in line with how we have handled that flag previously. I had a look at #48058 and it seems to the UX/UI part isn't figured out (which requires cross-team collab), so it feels too early to kick off actual development work on our side.

ywelsch · 2022-01-13T09:31:15Z

@elasticmachine run elasticsearch-ci/part-2

(known test failure: #82420)

javanna · 2022-01-13T10:25:53Z

On docs, I think there are bits that need to be updated besides what you mentioned around archive data.

See https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-index.html where we say that fields that are not indexed are not queryable. That is still true in general but not for numeric types. Also on https://www.elastic.co/guide/en/elasticsearch/reference/current/number.html#number-params we say that whether a number is searchable depends on whether it's indexed or not.

I would probably also update the bit on doc_values for numeric fields to mention that queries can exercise them in some cases, when they are not indexed, or when range queries are executed.

…-search

ywelsch · 2022-01-13T12:39:06Z

@javanna I've added some docs in 32c22fc. Let me know what you think.

javanna

Thanks for the docs changes, they look good to me. Last bit: should we also add some more info on the fact that doc_value queries are considered expensive where they are mentioned, and in query-dsl where the flag is mentioned? I see we have a (probably outdated) list of things that are considered expensive, I wonder if it's worth updating that.

javanna · 2022-01-13T13:35:38Z

docs/reference/mapping/params/doc-values.asciidoc

@@ -17,6 +17,12 @@ makes this data access pattern possible. They store the same values as the
 sorting and aggregations. Doc values are supported on almost all field types,
 with the __notable exception of `text` and `annotated_text` fields__.

+Numeric types, such as `long` and `double`, can also be queried when they are


may I ask why you mention only long and double?

Wasn't meant to be an exhaustive list. I've copied this phrasing from a different part of the docs. I've added a link on "Numeric types" now so that someone interested can look up all the types this applies to.

ywelsch · 2022-01-13T13:59:35Z

I've pushed some more doc changes in f9efb59.

Last bit: should we also add some more info on the fact that doc_value queries are considered expensive where they are mentioned,

Not sure what exactly what you want to see added. I mentioned everywhere that these queries are slow, and explained about the tradeoffs.

javanna · 2022-01-13T15:06:26Z

Not sure what exactly what you want to see added. I mentioned everywhere that these queries are slow, and explained about the tradeoffs.

What I meant is clarifying that the expensive queries flag affect these queries which may not be obvious. For instance for runtime fields we mention that queries on them are deemed expensive and can be disabled with such flag.

I am fine with adding doc_value queries to the list of things that are blocked with the flag though, like you did.

javanna

LGTM

ywelsch · 2022-01-13T15:23:14Z

Thanks @romseygeek and @javanna!

Similar to #82409, but for date fields. Allows searching on date field types (date, date_nanos) when those fields are not indexed (index: false) but just doc values are enabled. This enables searches on archive data, which has access to doc values but not index structures. When combined with searchable snapshots, it allows downloading only data for a given (doc value) field to quickly filter down to a select set of documents. Relates #81210 and #52728

jpountz · 2022-01-19T14:48:59Z

I added the release highlight label as this change is going to be quite powerful to speed up indexing and reduce storage requirements for fields that are typically not used for filtering.

Similar to #82409, but for geo_point fields. Allows searching on geo_point fields when those fields are not indexed (index: false) but just doc values are enabled. Also adds distance feature query support for date fields (bringing date field to feature parity with runtime fields) This enables searches on archive data, which has access to doc values but not index structures. When combined with searchable snapshots, it allows downloading only data for a given (doc value) field to quickly filter down to a select set of documents. Relates #81210 and #52728

Marks PR #82409 as a notable release highlight in its changelog YAML file. This ensures the highlight displays in the [Elastic Upgrade Guide](https://www.elastic.co/guide/en/elastic-stack/8.1/elasticsearch-highlights.html). Relates to #84041

ViggoC · 2022-03-30T08:56:04Z

server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java

+            if (isIndexed()) {
+                return type.termsQuery(name(), values);
+            } else {
+                return super.termsQuery(values, context);


Would DocValuesNumbersQuery be a better solution if there are so many terms?

Allow docvalues-only search on number types

85bc065

elasticsearchmachine added the v8.1.0 label Jan 11, 2022

ywelsch added 3 commits January 11, 2022 16:27

fix field caps gtest

c2b2869

fix more tests

166c6c1

Onore more test fix

359dc3c

ywelsch added :Search/Search Search-related issues that do not fall into other categories >enhancement labels Jan 11, 2022

ywelsch added 3 commits January 11, 2022 18:16

and another one

198d23f

another test fix

9a551dc

Merge remote-tracking branch 'elastic/master' into numeric-doc-values…

0217921

…-search

ywelsch marked this pull request as ready for review January 12, 2022 07:24

elasticmachine added the Team:Search Meta label for search team label Jan 12, 2022

ywelsch requested review from javanna and romseygeek January 12, 2022 07:25

romseygeek reviewed Jan 12, 2022

View reviewed changes

ywelsch added 3 commits January 12, 2022 13:45

Add expensive queries check

bae5818

Merge remote-tracking branch 'elastic/master' into numeric-doc-values…

b3c0cd7

…-search

fix tests...

5c4ad1b

romseygeek approved these changes Jan 12, 2022

View reviewed changes

ywelsch added 2 commits January 12, 2022 14:51

fix fix

e56c26b

another one

5135dae

javanna reviewed Jan 12, 2022

View reviewed changes

ywelsch added 3 commits January 13, 2022 09:33

update message

c2a9ca5

forceDocValues => isIndexed

4b273fd

Merge remote-tracking branch 'elastic/master' into numeric-doc-values…

d5fa293

…-search

ywelsch requested a review from javanna January 13, 2022 08:52

ywelsch added 2 commits January 13, 2022 13:20

Merge remote-tracking branch 'elastic/master' into numeric-doc-values…

9c1f11f

…-search

docs

32c22fc

javanna reviewed Jan 13, 2022

View reviewed changes

doc changes

f9efb59

javanna mentioned this pull request Jan 13, 2022

Doc-value-only fields #52728

Closed

javanna approved these changes Jan 13, 2022

View reviewed changes

ywelsch merged commit e421477 into elastic:master Jan 13, 2022

This was referenced Jan 14, 2022

Allow doc-values only search on date types #82602

Merged

Snapshots as simple archives #81210

Closed

This was referenced Jan 18, 2022

Exclude doc-values-only fields from '*' field expansion queries #82710

Closed

Exclude doc-value-only fields from all fields queries #82712

Closed

jpountz added the release highlight label Jan 19, 2022

This was referenced Jan 20, 2022

Allow doc-values only search on keyword fields #82846

Merged

Allow doc-values only search on boolean fields #82925

Merged

Allow doc-values only search on ip fields #82929

Merged

ywelsch mentioned this pull request Feb 2, 2022

Allow doc-values only search on geo_point fields #83395

Merged

jrodewig mentioned this pull request Feb 17, 2022

[DOCS] Mark PR #82409 as a notable release highlight #84103

Merged

ViggoC reviewed Mar 30, 2022

View reviewed changes

iverase mentioned this pull request Mar 8, 2023

Allow docvalues-only search on geo_shape #94396

Merged

not-napoleon mentioned this pull request Apr 3, 2023

Data histogram aggregation issue for arrays field #82903

Closed

Allow docvalues-only search on number types #82409

Allow docvalues-only search on number types #82409

Uh oh!

Conversation

ywelsch commented Jan 11, 2022 • edited by jpountz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jan 12, 2022

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywelsch commented Jan 13, 2022

Uh oh!

ywelsch commented Jan 13, 2022

Uh oh!

javanna commented Jan 13, 2022

Uh oh!

ywelsch commented Jan 13, 2022

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywelsch commented Jan 13, 2022

Uh oh!

javanna commented Jan 13, 2022

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

ywelsch commented Jan 13, 2022

Uh oh!

jpountz commented Jan 19, 2022

Uh oh!

Choose a reason for hiding this comment

ywelsch commented Jan 11, 2022 •

edited by jpountz

Loading