You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "waziqi89 (via GitHub)" <gi...@apache.org> on 2023/04/03 17:26:17 UTC
[GitHub] [lucene] waziqi89 opened a new pull request, #12222: fix FragEnd bug in BaseFragmentsBuilder
waziqi89 opened a new pull request, #12222:
URL: https://github.com/apache/lucene/pull/12222
### Description
This is a fix to https://github.com/apache/lucene/issues/12221 .
FragEnd was the start of the next potential fragment, which leading the boundary scanner to cover the first word in the next field when the fragment ends at current field's end.
Changed a text in `TestSimpleFragmentsBuilder.java`. The behavior change is expected (could be another bug):
1. the newly created fragments [will stop at the original (when discreteMultiValueHighlight is unset) fragment's end](https://github.com/waziqi89/lucene/blob/0c2f9a97af85c3839e0fe62fda90ba76925f8c31/lucene/highlighter/src/java/org/apache/lucene/search/vectorhighlight/BaseFragmentsBuilder.java#L313) even if the newly created fragment can be extended further as the new could fragment start later than the original one.
2. FragEnd used to be at "t" of the next word "text", so boundary scanner will cover the whole word. Now FragEnd stops before that character, so the "text" is excluded entirely
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] fix FragEnd bug in BaseFragmentsBuilder [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12222:
URL: https://github.com/apache/lucene/pull/12222#issuecomment-1880903470
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] waziqi89 commented on a diff in pull request #12222: fix FragEnd bug in BaseFragmentsBuilder
Posted by "waziqi89 (via GitHub)" <gi...@apache.org>.
waziqi89 commented on code in PR #12222:
URL: https://github.com/apache/lucene/pull/12222#discussion_r1167188703
##########
lucene/highlighter/src/test/org/apache/lucene/search/vectorhighlight/TestSimpleFragmentsBuilder.java:
##########
@@ -226,7 +226,7 @@ public void testDiscreteMultiValueHighlighting() throws Exception {
result = sfb.createFragments(reader, 0, F, ffl, 3);
assertEquals(2, result.length);
assertEquals("text to <b>highlight</b>", result[0]);
- assertEquals("<b>highlight</b> other text", result[1]);
+ assertEquals("<b>highlight</b> other", result[1]);
Review Comment:
Thanks @romseygeek for reviewing.
To be honest, I believe the current baseFragmentBuilder process a multivalue field wrongly in terms of maxCharSize.
For this test case, we have two value fields:
1. some text to highlight (22 chars)
2. highlight other text (20 chars)
Status quo (in main/master):
fragment 1: text to <b>highlight</b>
fragment 2: <b>highlight</b> other text
Please note that the fragment 1 excludes the first word "some", while there are 22 chars only in the entire first field. Even though I didn't spend too much time on how the multivalue fields are processed, I believe current logic is to treat the all values as a single value field, and construct the FragInfos; Then split it into multiple chunks if the Fragments are cross multiple fields. This behavior will cause the newly splitted fragments smaller than the original maxFragmentSize.
In this use case, my fix to the bug I aimed to fix (wrong fragEnd) only make the fragment splitting logic looks worse. But those are two different bugs.
If we want to change the splitting logics, a mini-rework is supposed to be done in this class, I am afraid.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] romseygeek commented on a diff in pull request #12222: fix FragEnd bug in BaseFragmentsBuilder
Posted by "romseygeek (via GitHub)" <gi...@apache.org>.
romseygeek commented on code in PR #12222:
URL: https://github.com/apache/lucene/pull/12222#discussion_r1164021939
##########
lucene/highlighter/src/test/org/apache/lucene/search/vectorhighlight/TestSimpleFragmentsBuilder.java:
##########
@@ -226,7 +226,7 @@ public void testDiscreteMultiValueHighlighting() throws Exception {
result = sfb.createFragments(reader, 0, F, ffl, 3);
assertEquals(2, result.length);
assertEquals("text to <b>highlight</b>", result[0]);
- assertEquals("<b>highlight</b> other text", result[1]);
+ assertEquals("<b>highlight</b> other", result[1]);
Review Comment:
Do you know why this has changed? The fragment length is still 32 characters so I'd expect to get the full text string back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] waziqi89 commented on a diff in pull request #12222: fix FragEnd bug in BaseFragmentsBuilder
Posted by "waziqi89 (via GitHub)" <gi...@apache.org>.
waziqi89 commented on code in PR #12222:
URL: https://github.com/apache/lucene/pull/12222#discussion_r1167188703
##########
lucene/highlighter/src/test/org/apache/lucene/search/vectorhighlight/TestSimpleFragmentsBuilder.java:
##########
@@ -226,7 +226,7 @@ public void testDiscreteMultiValueHighlighting() throws Exception {
result = sfb.createFragments(reader, 0, F, ffl, 3);
assertEquals(2, result.length);
assertEquals("text to <b>highlight</b>", result[0]);
- assertEquals("<b>highlight</b> other text", result[1]);
+ assertEquals("<b>highlight</b> other", result[1]);
Review Comment:
Thanks @romseygeek for reviewing.
To be honest, I believe the current baseFragmentBuilder process a multivalue field wrongly in terms of maxCharSize.
For this test case, we have two value fields:
1. some text to highlight (22 chars)
2. highlight other text (20 chars)
Status quo (in main/master):
fragment 1: text to <b>highlight</b>
fragment 2: <b>highlight</b> other text
Please note that the fragment 1 excludes the first word "some", while there are 22 chars only in the entire first field. Even though I didn't spend too much time on how the multivalue fields are processed, I believe current logic is to treat all values as a single value field, and construct the FragInfos; Then split fragments into multiple chunks if they are cross multiple fields. This behavior will cause the newly splitted fragments smaller than the desired maxFragmentSize if we don't expand them after splitting.
In this use case, my fix to the bug I aimed to fix (wrong fragEnd) only make the fragment splitting logic looks worse. But those are two different bugs.
If we want to change the splitting logics, a mini-rework is supposed to be done in this class, I am afraid.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] waziqi89 commented on a diff in pull request #12222: fix FragEnd bug in BaseFragmentsBuilder
Posted by "waziqi89 (via GitHub)" <gi...@apache.org>.
waziqi89 commented on code in PR #12222:
URL: https://github.com/apache/lucene/pull/12222#discussion_r1167188703
##########
lucene/highlighter/src/test/org/apache/lucene/search/vectorhighlight/TestSimpleFragmentsBuilder.java:
##########
@@ -226,7 +226,7 @@ public void testDiscreteMultiValueHighlighting() throws Exception {
result = sfb.createFragments(reader, 0, F, ffl, 3);
assertEquals(2, result.length);
assertEquals("text to <b>highlight</b>", result[0]);
- assertEquals("<b>highlight</b> other text", result[1]);
+ assertEquals("<b>highlight</b> other", result[1]);
Review Comment:
Thanks @romseygeek for reviewing.
To be honest, I believe the current baseFragmentBuilder process a multivalue field wrongly in terms of maxCharSize.
For this test case, we have two value fields:
1. some text to highlight (22 chars)
2. highlight other text (20 chars)
Status quo (in main/master):
fragment 1: text to <b>highlight</b>
fragment 2: <b>highlight</b> other text
Please note that the fragment 1 excludes the first word "some", while there are 22 chars only in the entire first field. Even though I didn't spend too much time on how the multivalue fields are processed, I believe current logic is to treat all values as a single value field, and construct the FragInfos; Then split fragments into multiple chunks if they are cross multiple fields. This behavior will cause the newly splitted fragments smaller than the desired maxFragmentSize if we don't expand them after splitting.
In this test case, my fix to the bug I aimed to fix (wrong fragEnd) only make the fragment splitting logic looks worse. But those are two different bugs.
If we want to change the splitting logics, a mini-rework is supposed to be done in this class, I am afraid.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] romseygeek commented on a diff in pull request #12222: fix FragEnd bug in BaseFragmentsBuilder
Posted by "romseygeek (via GitHub)" <gi...@apache.org>.
romseygeek commented on code in PR #12222:
URL: https://github.com/apache/lucene/pull/12222#discussion_r1164024337
##########
lucene/highlighter/src/test/org/apache/lucene/search/vectorhighlight/TestSimpleFragmentsBuilder.java:
##########
@@ -226,7 +226,7 @@ public void testDiscreteMultiValueHighlighting() throws Exception {
result = sfb.createFragments(reader, 0, F, ffl, 3);
assertEquals(2, result.length);
assertEquals("text to <b>highlight</b>", result[0]);
- assertEquals("<b>highlight</b> other text", result[1]);
+ assertEquals("<b>highlight</b> other", result[1]);
Review Comment:
Reading again, I see now that you covered this in your opening comment, sorry! I'm not sure it's the correct behaviour though. Maybe we need to take into account if this is the final possible text?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] waziqi89 commented on a diff in pull request #12222: fix FragEnd bug in BaseFragmentsBuilder
Posted by "waziqi89 (via GitHub)" <gi...@apache.org>.
waziqi89 commented on code in PR #12222:
URL: https://github.com/apache/lucene/pull/12222#discussion_r1167188703
##########
lucene/highlighter/src/test/org/apache/lucene/search/vectorhighlight/TestSimpleFragmentsBuilder.java:
##########
@@ -226,7 +226,7 @@ public void testDiscreteMultiValueHighlighting() throws Exception {
result = sfb.createFragments(reader, 0, F, ffl, 3);
assertEquals(2, result.length);
assertEquals("text to <b>highlight</b>", result[0]);
- assertEquals("<b>highlight</b> other text", result[1]);
+ assertEquals("<b>highlight</b> other", result[1]);
Review Comment:
Thanks @romseygeek for reviewing.
To be honest, I believe the current baseFragmentBuilder process a multivalue field wrongly in terms of maxCharSize.
For this test case, we have two value fields:
1. some text to highlight (22 chars)
2. highlight other text (20 chars)
Status quo (in main/master):
fragment 1: text to <b>highlight</b>
fragment 2: <b>highlight</b> other text
Please note that the fragment 1 excludes the first word "some", while there are 22 chars only in the entire first field. Even though I didn't spend too much time on how the multivalue fields are processed, I believe current logic is to treat all values as a single value field, and construct the FragInfos; Then split fragments into multiple chunks if they are cross multiple fields. This behavior will cause the newly splitted fragments smaller than the desired maxFragmentSize if we don't expand them after splitting.
In this test case, my fix to the bug I aimed to fix (wrong fragEnd) only makes the fragment splitting logic looks worse and more obvious. But those are two different bugs.
If we want to change the splitting logics, a mini-rework is supposed to be done in this class, I am afraid.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] fix FragEnd bug in BaseFragmentsBuilder [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12222:
URL: https://github.com/apache/lucene/pull/12222#issuecomment-3413343885
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org