Exclude doc-value-only fields from all fields queries

[romseygeek]

Doc-value-only fields can be searched (slowly) by doing a table scan; this
commit ensures that these fields are not included in an all-field query, ie
a query_string or similar query against *, to prevent these queries
becoming unreasonably slow.

Fixes #82710

[elasticmachine]

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

[ywelsch]

LGTM

[romseygeek]

Aha, the failing tests are because this new restriction will also exclude runtime fields. Which is an interesting question - do we want to exclude them, given that they also effectively do a table scan and are therefore very slow? Or do we want to be cleverer here?

[ywelsch]

I think runtime fields should be excluded as well. If not, then I don't think doc-value-only fields should be excluded either, as runtime fields are generally even slower than doc-value-only fields.

[romseygeek]

I think this is a bug in the query parsing code, or at least an inconsistency. According to #25726, we expand fields in the following ways:

• If the field expansion is a partial wildcard, eg foo* then we only expand to searchable fields
• If the field expansion is total, ie *, then we only expand to searchable fields AND we exclude metadata fields.

But what actually happens seems to happen for query_string:

• If the field expansion is partial then we exclude metadata fields but don't filter out anything else
• If the field expansion is total then we filter out non-searchable fields and metadata fields

In other words, partial field expansions are excluding the wrong thing. On simple_query_string we do what the original issue intended.

I'll change the doc-values (and runtime fields) exclusion to work on *, which I think is the original intent. But we should probably fix query_string separately.

[ywelsch]

I've left one comment. While I understand what the intent behind the current logic is, I find it highly confusing how it is currently represented / named.

[ywelsch]

The acceptMetadataField variable name is highly confusing if we're using it here to denote full vs partial wilcard. The javadocs for acceptMetadataField say "Whether metadata fields should be added when a pattern is expanded.". Non-indexed fields shouldn't be added though, independently of whether they're metadata fields or not? Which metadata fields exist that are not indexed? I find this code now highly confusing, as the relation between indexed and acceptMetadataField isn't clear, at least with how things are currently named.

[jpountz]

Do I understand correctly that a doc-value-only field would be excluded from * but not from *_foo? I wonder if it would be confusing to users.

[romseygeek]

Do I understand correctly that a doc-value-only field would be excluded from * but not from *_foo? I wonder if it would be confusing to users.

Yes, and I agree that it's not ideal, but there's precedent in that this is what we already do for metadata fields. I'm mainly concerned that after #82409 kibana searches may get much slower because all of sudden we're doing a table scan for every query

[jpountz]

Thinking out loud, I wonder if there are other ways how we could do this, such as using a placeholder like _default that consists of either index.query.default_field if set, or the list of fields that are reasonable to use in a query with the approach that your PR is suggesting, and keeping the current semantics of *.

[romseygeek]

Thinking out loud, I wonder if there are other ways how we could do this, such as using a placeholder like _default that consists of either index.query.default_field if set, or the list of fields that are reasonable to use in a query with the approach that your PR is suggesting, and keeping the current semantics of *.

So if you explicitly set * as your field then you get everything, but if you don't set anything then we fall back to index.query.default_field, and if that isn't set then we fall back to the reduced list?

[jpountz]

Oh yes, something like that sounds nice!

[elasticsearchmachine]

Hi @romseygeek, I've created a changelog YAML for you.

[javanna]

Closing, we decided not to proceed with this change. See a summary of the discussion at #82710