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

Use default_field in highlight_query instead of all_fields #13185

Closed
wants to merge 1 commit into from

Conversation

lukasolson
Copy link
Member

Fixes #13096.

This PR changes the behavior of highlighting to use "default_field": "*" instead of "all_fields": true.

See #9671 for the original PR.

@Bargs
Copy link
Contributor

Bargs commented Jul 28, 2017

There may be a bug in ES, or default_field: * may not be an exact replacement for all_fields after all. I'm getting the following error when I search for a number with no field specified:

screen shot 2017-07-28 at 5 04 35 pm

Here's what my highlight query looked like:

  "highlight": {
    "pre_tags": [
      "@kibana-highlighted-field@"
    ],
    "post_tags": [
      "@/kibana-highlighted-field@"
    ],
    "fields": {
      "*": {
        "highlight_query": {
          "bool": {
            "must": [
              {
                "query_string": {
                  "query": "200",
                  "analyze_wildcard": true,
                  "default_field": "*"
                }
              },
              {
                "range": {
                  "@timestamp": {
                    "gte": 1498684188295,
                    "lte": 1501276188295,
                    "format": "epoch_millis"
                  }
                }
              }
            ],
            "filter": [],
            "should": [],
            "must_not": []
          }
        }
      }
    },
    "fragment_size": 2147483647
  }

@Bargs
Copy link
Contributor

Bargs commented Jul 28, 2017

I also had another related thought. I wonder if we should take the opportunity here to set default_field: * as the default in query:queryString:options and just get rid of this doc_table:highlight:all_fields setting entirely. If I remember correctly we only used the highlight query in 5.x because forcing all_fields on all indices would technically be a breaking change. In 6.0 we could make that change and just delete all this code. Users who have a default_field set in their index patterns could just update query:queryString:options.

Am I missing any reason why we wouldn't want to do that?

@lukasolson
Copy link
Member Author

@Bargs That's a good point. It would be a breaking change for 6.0 since some users would have to update the setting as you mentioned, but I think it would be super nice to get rid of this code anyway. It all feels pretty hacky to me.

@lukasolson
Copy link
Member Author

There are two kinds of users that might have to update their advanced settings if we do that: Those who have a default_field set in their index, and those who have, in the past, already modified the query:queryString:options advanced setting (to include the default_field parameter). Is that right?

@Bargs
Copy link
Contributor

Bargs commented Jul 31, 2017

and those who have, in the past, already modified the query:queryString:options advanced setting (to include the default_field parameter).

I suppose that's true, yeah.

@Bargs
Copy link
Contributor

Bargs commented Jul 31, 2017

@lukasolson I re-tested with a build of the most recent commit in ES master and I can no longer reproduce that error, so perhaps it was a bug that's already been fixed. This LGTM. If you want we could merge this as-is to avoid spamming users' deprecation logs and consider removal as a separate issue.

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work as expected against elastic/elasticsearch@315319b.

@lukasolson lukasolson closed this Aug 4, 2017
@lukasolson lukasolson deleted the default_field branch March 27, 2018 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants