Add ability to set "max_analyzed_offset" implicitly to "index.highlight #118895

svilen-mihaylov-elastic · 2024-12-17T21:45:26Z

Add ability to set "max_analyzed_offet" implicitly to "index.highlight
.max_analyzed_offset", by setting it excplicitly to "-1".

Closes #112822

.max_analyzed_offset", by setting it excplicitly to "-1".

elasticsearchmachine · 2024-12-18T22:23:54Z

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

john-wagster

LGTM, I agree (based on our discussion earlier today) some additional testing would be good. So probably worth a +1 from Mayya when she returns. Probably worth backporting this to v8.18.0 as well; I've thrown that label on the PR and thrown on the auto-back label for when you merge this. Feel free to take those off if you have questions or concerns for any reason.

svilen-mihaylov-elastic · 2025-01-02T16:42:44Z

...lClusterTest/java/org/elasticsearch/search/fetch/subphase/highlight/HighlighterSearchIT.java

+    public void testMaxQueryOffsetDefault() throws Exception {
+        assertAcked(
+            prepareCreate("test").setMapping(type1PostingsffsetsMapping())
+                .setSettings(Settings.builder().put("index.highlight.max_analyzed_offset", "10").build())


Observe index offset is 10

svilen-mihaylov-elastic · 2025-01-02T16:42:56Z

...lClusterTest/java/org/elasticsearch/search/fetch/subphase/highlight/HighlighterSearchIT.java

+        logger.info("--> highlighting and searching on field1");
+        // Specific for this test: by passing "-1" as "maxAnalyzedOffset", the index highlight setting above will be used.
+        SearchSourceBuilder source = searchSource().query(termQuery("field1", "sentence"))
+            .highlighter(highlight().field("field1").order("score").maxAnalyzedOffset(-1));


Observe query offset is -1

svilen-mihaylov-elastic · 2025-01-02T16:43:05Z

...lClusterTest/java/org/elasticsearch/search/fetch/subphase/highlight/HighlighterSearchIT.java

+            assertThat(field1.fragments().length, equalTo(1));
+            assertThat(
+                field1.fragments()[0].string(),
+                equalTo("This <em>sentence</em> contains one match, not that short. This sentence contains zero sentence matches.")


Observe only one match

svilen-mihaylov-elastic · 2025-01-02T22:40:19Z

docs/reference/search/search-your-data/highlighting.asciidoc

-no error is returned. The <<max-analyzed-offset, `max_analyzed_offset`>> query setting
+no error is returned. If it is set to -1 then the value of
+<<index-max-analyzed-offset, `index.highlight.max_analyzed_offset`>> is used instead.
+For values <= -1, an error is returned. The <<max-analyzed-offset, `max_analyzed_offset`>> query setting


The formatting here doesnt seem affected by spotlessApply, is there another tool to lint?

svilen-mihaylov-elastic · 2025-01-02T23:55:41Z

Looking at the CI tests, it looks like the backwards compatibility tests are failing since I've changed the exception message (https://buildkite.com/elastic/elasticsearch-pull-request/builds/48659#0194292d-5312-4ee2-a6f2-a3217e270df8). Also more generally we do not throw when the value is -1 now. This seems to be running against some older branch....

What would be the way to handle this? Should we backport this commit?

mayya-sharipova

@svilen-mihaylov-elastic Thanks for a great work!

server/src/main/java/org/elasticsearch/rest/action/search/SearchCapabilities.java

svilen-mihaylov-elastic · 2025-01-06T22:05:20Z

@elasticsearchmachine test this please

svilen-mihaylov-elastic · 2025-01-06T22:42:40Z

@elasticsearchmachine test this please

elasticsearchmachine · 2025-01-07T16:21:17Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 118895

…ht (elastic#118895) Add ability to set "max_analyzed_offet" implicitly to "index.highlight .max_analyzed_offset", by setting it excplicitly to "-1". Closes elastic#112822 (cherry picked from commit 93c349c)

svilen-mihaylov-elastic · 2025-01-30T19:01:13Z

💚 All backports created successfully

Status	Branch	Result
✅	8.18

Questions ?

Please refer to the Backport tool documentation

…ht (#118895) (#121323) Add ability to set "max_analyzed_offet" implicitly to "index.highlight .max_analyzed_offset", by setting it excplicitly to "-1". Closes #112822 (cherry picked from commit 93c349c)

elasticsearchmachine added needs:triage Requires assignment of a team area label v9.0.0 labels Dec 17, 2024

svilen-mihaylov-elastic force-pushed the svilen-mihaylov/112822 branch 3 times, most recently from 0f3e7c9 to cdc01a6 Compare December 18, 2024 15:37

svilen-mihaylov-elastic changed the title ~~Add ability to set "max_analyzed_offet" implicitly to "index.highlight~~ Add ability to set "max_analyzed_offset" implicitly to "index.highlight Dec 18, 2024

svilen-mihaylov-elastic force-pushed the svilen-mihaylov/112822 branch 4 times, most recently from 8fe10d5 to 11e82d3 Compare December 18, 2024 20:51

Add ability to set "max_analyzed_offet" implicitly to "index.highlight

2faea54

.max_analyzed_offset", by setting it excplicitly to "-1".

svilen-mihaylov-elastic force-pushed the svilen-mihaylov/112822 branch from 11e82d3 to 2faea54 Compare December 18, 2024 22:13

svilen-mihaylov-elastic added the Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch label Dec 18, 2024

elasticsearchmachine removed the Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch label Dec 18, 2024

svilen-mihaylov-elastic added :Search Relevance/Suggesters "Did you mean" and suggestions as you type Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch labels Dec 18, 2024

elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Dec 18, 2024

john-wagster added >bug >feature >enhancement and removed >bug >feature labels Dec 19, 2024

john-wagster approved these changes Dec 19, 2024

View reviewed changes

john-wagster added v8.18.0 auto-backport Automatically create backport pull requests when merged labels Dec 19, 2024

Add test

f26f2c7

svilen-mihaylov-elastic commented Jan 2, 2025

View reviewed changes

Added test for zero

eb6725e

svilen-mihaylov-elastic commented Jan 2, 2025

View reviewed changes

svilen-mihaylov-elastic added 3 commits January 3, 2025 11:50

Add a capability

4ae0be1

Add test for -1

c0c2127

Exclude test from REST compatibility

c6d2920

mayya-sharipova approved these changes Jan 3, 2025

View reviewed changes

mayya-sharipova reviewed Jan 3, 2025

View reviewed changes

server/src/main/java/org/elasticsearch/rest/action/search/SearchCapabilities.java Outdated Show resolved Hide resolved

svilen-mihaylov-elastic added 10 commits January 3, 2025 17:57

Add Highlight_ to search capability name

703f438

Merge branch 'main' into svilen-mihaylov/112822

bf90d9a

Disallow 0

9fc0fcb

Fix test

6fd85f3

Should SUCCEED -> Should FAIL

5abd2c4

Oops wrong test

0867480

Rename another test

f889a61

Remove test with =0 for now.

dc54b64

Restore test with zero

909d90f

Merge branch 'main' into svilen-mihaylov/112822

7337f1d

svilen-mihaylov-elastic added 2 commits January 7, 2025 10:09

Merge branch 'main' into svilen-mihaylov/112822

13cda1e

Fix merge

15bc509

svilen-mihaylov-elastic merged commit 93c349c into main Jan 7, 2025
17 checks passed

svilen-mihaylov-elastic deleted the svilen-mihaylov/112822 branch January 7, 2025 16:19

elasticsearchmachine added the backport pending label Jan 7, 2025

svilen-mihaylov-elastic mentioned this pull request Jan 30, 2025

[8.18] Add ability to set "max_analyzed_offset" implicitly to "index.highlight (#118895) #121323

Merged

Add ability to set "max_analyzed_offset" implicitly to "index.highlight #118895

Add ability to set "max_analyzed_offset" implicitly to "index.highlight #118895

Uh oh!

Conversation

svilen-mihaylov-elastic commented Dec 17, 2024 • edited by mayya-sharipova Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Dec 18, 2024

Uh oh!

john-wagster left a comment

Choose a reason for hiding this comment

Uh oh!

svilen-mihaylov-elastic Jan 2, 2025

Choose a reason for hiding this comment

Uh oh!

svilen-mihaylov-elastic Jan 2, 2025

Choose a reason for hiding this comment

Uh oh!

svilen-mihaylov-elastic Jan 2, 2025

Choose a reason for hiding this comment

Uh oh!

svilen-mihaylov-elastic Jan 2, 2025

Choose a reason for hiding this comment

Uh oh!

svilen-mihaylov-elastic commented Jan 2, 2025

Uh oh!

mayya-sharipova left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

svilen-mihaylov-elastic commented Jan 6, 2025

Uh oh!

svilen-mihaylov-elastic commented Jan 6, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Jan 7, 2025

💔 Backport failed

Uh oh!

svilen-mihaylov-elastic commented Jan 30, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

svilen-mihaylov-elastic commented Dec 17, 2024 •

edited by mayya-sharipova

Loading

mayya-sharipova left a comment •

edited

Loading