Add LimitedOffsetsEnum to Limited offset token #86110

luyuncheng · 2022-04-24T10:05:24Z

In #67325 add max_analyzed_offset allows users to limit the highlighting of text fields.
but when using "index_options": "offsets" in mappings, this max_analyzed_offset offset can not be limited,
because it would use OffsetSource.ANALYSISand PostingsOffsetStrategy to highlight field.

When a string="Testing Fun Testing Fun" need to be highlight

Using UnifiedHighlighter.OffsetSource.ANALYSIS with queryMaxAnalyzedOffset=10 would response:

"Testing <b>Fun</b> Testing Fun"

because in #67325 add LimitTokenOffsetAnalyzer

BUT USING UnifiedHighlighter.OffsetSource.POSTINGS which mapping is index_options=offsets and queryMaxAnalyzedOffset=10 would response:

"Testing <b>Fun</b> Testing <b>Fun</b>"

because offset can not be limited by LimitTokenOffsetAnalyzer

may be can add a new LimitedOffsetsEnum to limit offset token

Link Issue: #86109

2. Add highlight tests for maxAnalyzedOffset in different OffsetSource

elasticmachine · 2022-05-18T20:11:28Z

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

rahuldimri · 2022-08-11T03:04:09Z

is this issue available to work ?. or is there anything pending on this issue?

romseygeek · 2022-09-23T14:39:00Z

.../src/test/java/org/elasticsearch/lucene/search/uhighlight/CustomUnifiedHighlighterTests.java

+
+
+    protected Analyzer wrapAnalyzer(Analyzer analyzer, Integer maxAnalyzedOffset) {
+        if (maxAnalyzedOffset != null) {


Why do you need to wrap the analyzer here? Shouldn't it be handled by passing the max offset to CustomUnifiedHighlighter?

At beginning i want to describe how the analyzer runs with different analyzers(Default VS LimitTokenOffsetAnalyzer) works in different maxAnalyzedOffset.
i think in this test case is unnecessary, but we can add random tests for different maxAnalyzedOffset

++, random tests would be great

romseygeek · 2022-10-14T13:05:42Z

@elasticmachine test this please

…omUnifiedHighlighterTests

luyuncheng · 2022-10-18T04:35:45Z

++, random tests would be great

@romseygeek Thanks for the reviewing this.

At 5d5e701 i removed the wrapAnalyzer and merge the assertHighlightOneDoc into one function
AND
i added a test for random different OffsetSource and maxAnalyzedOffset

romseygeek

Thanks @luyuncheng! I left a few small comments but I think this is close.

romseygeek · 2022-10-18T07:37:00Z

server/src/main/java/org/elasticsearch/lucene/search/uhighlight/LimitedOffsetsEnum.java

+    private final OffsetsEnum delegate;
+    private final Integer maxOffset;
+
+    public LimitedOffsetsEnum(OffsetsEnum delegate, @Nullable Integer maxOffset) {


I would have this as a raw int rather than an object, and just don't wrap with this Enum if the max offset is not defined. Then we can remove the null check on every nextPosition call.

In fact you already have a null check at the call site so we can happily make this an int

In e18513f fixed it

romseygeek · 2022-10-18T07:44:25Z

.../src/test/java/org/elasticsearch/lucene/search/uhighlight/CustomUnifiedHighlighterTests.java

+            Locale.ROOT,
+            BreakIterator.getSentenceInstance(Locale.ROOT),
+            0,
+            // OLD Strategy would Response： "Testing <b>Fun</b> Testing <b>Fun</b>"


This comment will be confusing in a couple of years time (what was the old strategy? why was it different?). I think the current behaviour is a bug so it's fine to just test for it, no need to add a comment.

romseygeek · 2022-10-18T07:47:04Z

server/src/main/java/org/elasticsearch/lucene/search/uhighlight/CustomFieldHighlighter.java

    @Override
    protected Passage[] highlightOffsetsEnums(OffsetsEnum off) throws IOException {

+        OffsetsEnum wrapOff = off;


nit: no need to have a new variable here, you can just reassign off

romseygeek · 2022-10-18T10:38:21Z

@elasticmachine test this please

romseygeek · 2022-10-18T10:44:15Z

@elasticmachine generate changelog

romseygeek · 2022-10-18T10:46:03Z

docs/changelog/86110.yaml

@@ -0,0 +1,6 @@
+pr: 86110
+summary: Add LimitedOffsetsEnum to Limited offset token
+area: Search/Highlighting


Can you change this to just Search? Apparently Search/Highlighting doesn't work here, 🤷

Fixed chagelog area

romseygeek · 2022-10-18T11:04:33Z

@elasticmachine ok to test

romseygeek · 2022-10-18T11:18:18Z

Hi @luyuncheng can you run ./gradlew precommit at the root and then push the relevant changes?

Merge Master into it

…TOKENS_PRE Merge Master and precommit into it

romseygeek · 2022-10-18T14:35:08Z

@elasticmachine update branch

romseygeek · 2022-10-18T15:30:46Z

Thanks for your patience @luyuncheng!

1. Add LimitedOffsetsEnum to Limited offset token

87fe7aa

2. Add highlight tests for maxAnalyzedOffset in different OffsetSource

elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v8.3.0 labels Apr 24, 2022

luyuncheng mentioned this pull request Apr 24, 2022

max_analyzed_offset can not Limited OffsetSource.POSTINGS #86109

Closed

jtibshirani added the :Search Relevance/Highlighting How a query matched a document label May 18, 2022

elasticmachine added the Team:Search Meta label for search team label May 18, 2022

craigtaverner added v8.4.0 and removed v8.3.0 labels May 25, 2022

javanna removed the v8.4.0 label Jun 1, 2022

elasticsearchmachine changed the base branch from master to main July 22, 2022 23:07

romseygeek reviewed Sep 23, 2022

View reviewed changes

jtibshirani assigned romseygeek Sep 23, 2022

luyuncheng added 2 commits October 17, 2022 16:22

Merge branch 'elastic:main' into LIMIT_CustomUnifiedHighlighter_TOKENS

1a009ea

1. Add changelog AND Random tests for different offset source in Cust…

5d5e701

…omUnifiedHighlighterTests

luyuncheng requested a review from romseygeek October 18, 2022 04:35

romseygeek requested changes Oct 18, 2022

View reviewed changes

luyuncheng added 2 commits October 18, 2022 18:07

Fixed CustomFieldHighlighter for param AND remove some comment

e18513f

Remove unnecessary import

c4d8524

luyuncheng requested a review from romseygeek October 18, 2022 10:35

romseygeek added v8.6.0 >enhancement labels Oct 18, 2022

romseygeek reviewed Oct 18, 2022

View reviewed changes

Update 86110.yaml

8e9b8c2

Fixed chagelog area

luyuncheng requested a review from romseygeek October 18, 2022 10:50

Merge branch 'elastic:main' into LIMIT_CustomUnifiedHighlighter_TOKENS

e01ebd4

luyuncheng added 3 commits October 18, 2022 21:36

Merge pull request #2 from luyuncheng/master

3a82ed1

Merge Master into it

precommit check

3eca380

Merge pull request #3 from luyuncheng/LIMIT_CustomUnifiedHighlighter_…

39a9035

…TOKENS_PRE Merge Master and precommit into it

Merge branch 'main' into LIMIT_CustomUnifiedHighlighter_TOKENS

55d3b38

romseygeek approved these changes Oct 18, 2022

View reviewed changes

romseygeek merged commit f641000 into elastic:main Oct 18, 2022



		protected Analyzer wrapAnalyzer(Analyzer analyzer, Integer maxAnalyzedOffset) {
		if (maxAnalyzedOffset != null) {

Add LimitedOffsetsEnum to Limited offset token #86110

Add LimitedOffsetsEnum to Limited offset token #86110

Uh oh!

Conversation

luyuncheng commented Apr 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented May 18, 2022

Uh oh!

rahuldimri commented Aug 11, 2022

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 commented Oct 14, 2022

Uh oh!

luyuncheng commented Oct 18, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek commented Oct 18, 2022

Uh oh!

romseygeek commented Oct 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek commented Oct 18, 2022

Uh oh!

romseygeek commented Oct 18, 2022

Uh oh!

romseygeek commented Oct 18, 2022

Uh oh!

romseygeek commented Oct 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

luyuncheng commented Apr 24, 2022 •

edited

Loading