Use ValueFetcher when loading text snippets to highlight #63572

romseygeek · 2020-10-12T18:00:56Z

HighlighterUtils.loadFieldValues() loads values directly from the source, and
then callers have to deal with filtering out values that would have been removed
by an ignore_above filter on keyword fields. Instead, we can use the
ValueFetcher for the relevant field, which handles all this logic for us.

Closes #59931.

elasticmachine · 2020-10-12T18:00:58Z

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

nik9000 · 2020-10-12T18:24:31Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightUtils.java

-        return textsToHighlight;
+                                               MapperService mapperService,
+                                               FetchSubPhase.HitContext hitContext) throws IOException {
+        ValueFetcher fetcher = fieldType.valueFetcher(mapperService, null, null);


null looks is going to cause this to fail on runtime fields. Do we filter those out other places?

I meant to highlight this with a comment of 'not sure about this' - I'm pretty sure that we filter out non-text fields further up the stack, but I need to double-check.

I think we do try to highlight runtime fields. I'm not sure how we manage not to fail here. Runtime fields really need the SearchLookup to do anything.

I vaguely remember looking at this too, and finding that for runtime fields we simply don't return anything highlighted rather than failing, as they are not in _source nor stored.

Given that runtime fields can't be highlighted anyway, I just added a null check for SearchLookup inside the value fetcher in AbstractScriptFieldType.

server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightUtils.java

jtibshirani

It's great to see the value fetcher abstraction simplify some special cases! One general comment -- now that we use value fetchers, we will highlight copy_to content. (For multi-fields, I think they were already handled before this change.) Perhaps we could add tests for this enhancement and make it more visible in the version 'changes' docs?

jtibshirani · 2020-11-10T23:22:02Z

@romseygeek just a heads up that I marked this PR as closing #59931.

romseygeek · 2020-11-19T15:04:48Z

We have an interesting test failure here:

org.elasticsearch.search.fetch.subphase.highlight.HighlighterSearchIT > testWithNormalizer FAILED |  
-- | --
  | java.lang.AssertionError: |  
  | Expected: "<em>Hello World</em>" |  
  | but: was "<em>hello world</em>" |  
  | at __randomizedtesting.SeedInfo.seed([6429DCC7E15891CE:7E8A44B3C85F94F]:0) |  
  | at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18) |  
  | at org.junit.Assert.assertThat(Assert.java:956)

Keyword value fetchers normalize their data, but this highlighter test is expecting normalization not to be applied to the highlighter output. I'm thinking we should change the test, as it makes more sense for the behaviour to be consistent across fields and highlights.

romseygeek · 2020-11-19T15:06:19Z

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

@@ -60,7 +60,7 @@ public SourceValueFetcher(String fieldName, QueryShardContext context, Object nu
        for (String path : sourcePaths) {
            Object sourceValue = lookup.extractValue(path, nullValue);
            if (sourceValue == null) {
-                return List.of();
+                continue;


This was a bug - if the first of multiple source paths didn't have any values, we returned early instead of continuing to check further paths.

Thank you for catching this! Maybe we could fix this in a separate PR (with a quick test) to keep this one scoped to highlighting?

romseygeek · 2020-11-19T15:09:01Z

...elds/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptFieldType.java

@@ -198,6 +199,9 @@ protected final void checkAllowExpensiveQueries(QueryShardContext context) {

    @Override
    public ValueFetcher valueFetcher(QueryShardContext context, SearchLookup lookup, String format) {
+        if (lookup == null) {
+            return v -> Collections.emptyList();
+        }


Another option here would be to pass the Fetch-phase search lookup through all the highlighting code to HighlightUtils.loadValues(), and I think we probably will want to do that eventually, but it's not really ready yet given our uncertainty around how that lookup is built and passed around anyway.

I was going to ask why it is null, the supplier could throw unsupported operation exception and not be null, but it is no supplier in this case. I am not liking that much that it can be null, but I am not sure how we can avoid that in this scenario.

jtibshirani · 2020-11-19T20:50:11Z

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

@@ -60,7 +60,7 @@ public SourceValueFetcher(String fieldName, QueryShardContext context, Object nu
        for (String path : sourcePaths) {
            Object sourceValue = lookup.extractValue(path, nullValue);
            if (sourceValue == null) {
-                return List.of();
+                continue;


Thank you for catching this! Maybe we could fix this in a separate PR (with a quick test) to keep this one scoped to highlighting?

jtibshirani · 2020-11-19T20:59:12Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightUtils.java

-        //percolator needs to always load from source, thus it sets the global force source to true
-        List<Object> textsToHighlight;
-        if (forceSource == false && fieldType.isStored()) {
+                                               boolean storedFieldsAvailable) throws IOException {


To me it'd be best to keep the original forceSource name and value -- it matches the force_source REST option that this value comes and avoids having mixed concepts in the code.

jtibshirani · 2020-11-20T03:04:33Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightUtils.java

        }
-        assert textsToHighlight != null;
-        return textsToHighlight;
+        ValueFetcher fetcher = fieldType.valueFetcher(qsc, null, null);


I just opened #65292 -- if we like the direction and it gets merged, then MappedFieldType#valueFetcher won't need a dedicated SearchLookup at all. I think highlighting on runtime fields would 'just work' here (although it'd be worth a test!)

romseygeek · 2020-11-23T16:04:12Z

I opened #65375 to fix the copy_to fields bug separately.

jtibshirani

The change looks great to me. Some last high-level comments:

We're now able to highlight runtime keyword fields. It could also be good to add a REST test for runtime keywords, as I don't think we have any tests for this yet. And I think we can remove the highlight test exclusion from xpack/plugin/runtime-fields/qa/build.gradle.
This PR makes some changes to the default highlighting behavior: we now include copy_to fields, and keywords are normalized. We could make a small note in the migration docs about these changes?

romseygeek · 2020-11-24T10:39:50Z

We're now able to highlight runtime keyword fields

Unfortunately this doesn't quite work yet, because the highlighters try to re-analyze the values from source, and trying to pull an index analyzer for runtime fields will fail - @jimczi found this elsewhere. I'll try and fix this in a follow up, I think it's just a case of amending QueryShardContext.getFieldNameIndexAnalyzer()

We could make a small note in the migration docs about these changes?

++, will push a change for this

romseygeek · 2020-11-24T10:58:38Z

++, will push a change for this

Actually this will have to wait for the backport, as there's no 7.11 migration doc in master

HighlighterUtils.loadFieldValues() loads values directly from the source, and then callers have to deal with filtering out values that would have been removed by an ignore_above filter on keyword fields. Instead, we can use the ValueFetcher for the relevant field, which handles all this logic for us. Closes elastic#59931.

romseygeek · 2020-11-25T16:02:27Z

For 7.11 release notes: This change means that you can highlight fields populated via copy_to directives.

…5441) HighlighterUtils.loadFieldValues() loads values directly from the source, and then callers have to deal with filtering out values that would have been removed by an ignore_above filter on keyword fields. Instead, we can use the ValueFetcher for the relevant field, which handles all this logic for us. Closes #59931.

Use ValueFetcher when loading text snippets to highlight

48bf4b6

romseygeek added :Search/Highlighting How a query matched a document >refactoring v8.0.0 v7.11.0 labels Oct 12, 2020

romseygeek requested a review from nik9000 October 12, 2020 18:00

romseygeek self-assigned this Oct 12, 2020

elasticmachine added the Team:Search Meta label for search team label Oct 12, 2020

nik9000 reviewed Oct 12, 2020

View reviewed changes

jtibshirani reviewed Oct 12, 2020

View reviewed changes

romseygeek added 2 commits October 13, 2020 08:58

Add back stored-fields branch

36469e3

Merge remote-tracking branch 'origin/master' into highlight/fetcher

972a635

romseygeek mentioned this pull request Nov 6, 2020

Add runtime_mappings to search request #64374

Merged

romseygeek added 3 commits November 19, 2020 14:19

Merge remote-tracking branch 'origin/master' into highlight/fetcher

5b72b8b

Check for nulls in tokencount

a3baf24

test for copy_to highlighting; check *all* source paths

1c28877

romseygeek commented Nov 19, 2020

View reviewed changes

jtibshirani reviewed Nov 20, 2020

View reviewed changes

jtibshirani mentioned this pull request Nov 20, 2020

Simplify how source is passed to fetch subphases. #65292

Merged

romseygeek added 3 commits November 23, 2020 15:16

Merge remote-tracking branch 'origin/master' into highlight/fetcher

2ea506d

Call it forceSource again

0b3ecee

change test expectation

70e6880

jtibshirani reviewed Nov 23, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into highlight/fetcher

dc75855

jtibshirani approved these changes Nov 24, 2020

View reviewed changes

romseygeek merged commit d088171 into elastic:master Nov 24, 2020

romseygeek deleted the highlight/fetcher branch November 24, 2020 16:09

romseygeek added the >feature label Nov 25, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

jtibshirani mentioned this pull request Jan 18, 2022

Highlighting on a keyword field not working as expected #82694

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ValueFetcher when loading text snippets to highlight #63572

Use ValueFetcher when loading text snippets to highlight #63572

romseygeek commented Oct 12, 2020 •

edited by jtibshirani

elasticmachine commented Oct 12, 2020

nik9000 Oct 12, 2020

romseygeek Oct 12, 2020

nik9000 Oct 13, 2020

javanna Oct 14, 2020

romseygeek Nov 19, 2020

jtibshirani left a comment

jtibshirani commented Nov 10, 2020

romseygeek commented Nov 19, 2020

romseygeek Nov 19, 2020

jtibshirani Nov 19, 2020

romseygeek Nov 19, 2020

javanna Nov 19, 2020

jtibshirani Nov 19, 2020

jtibshirani Nov 19, 2020

jtibshirani Nov 20, 2020

romseygeek commented Nov 23, 2020

jtibshirani left a comment •

edited

romseygeek commented Nov 24, 2020

romseygeek commented Nov 24, 2020

romseygeek commented Nov 25, 2020

Use ValueFetcher when loading text snippets to highlight #63572

Use ValueFetcher when loading text snippets to highlight #63572

Conversation

romseygeek commented Oct 12, 2020 • edited by jtibshirani

elasticmachine commented Oct 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani left a comment

Choose a reason for hiding this comment

jtibshirani commented Nov 10, 2020

romseygeek commented Nov 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romseygeek commented Nov 23, 2020

jtibshirani left a comment • edited

Choose a reason for hiding this comment

romseygeek commented Nov 24, 2020

romseygeek commented Nov 24, 2020

romseygeek commented Nov 25, 2020

romseygeek commented Oct 12, 2020 •

edited by jtibshirani

jtibshirani left a comment •

edited