Improve efficiency of BoundedBreakIteratorScanner fragmentation algorithm (the 2nd) #75306
Conversation
elasticsearchmachine
added
v8.0.0
external-contributor
|
@elasticmachine ok to test |
There was a problem hiding this comment.
Thanks for fixing the overflow and adding the test @thelink2012.
We should be more strict with the range of passage size that we accept but that's outside of the scope of this PR. I'll merge soon.
|
Hey Jim,
Thanks for looking. Though this code isn't ready for merging. This still
produces some failures in Kibana's test suite.
I'd be grateful if you could take a look at these, since I had some trouble
setting up Kibana in order to reproduce.
Best
…On Mon, Jul 26, 2021, 12:44 PM Jim Ferenczi ***@***.***> wrote:
***@***.**** approved this pull request.
Thanks for fixing the overflow and adding the test @thelink2012
<https://github.com/thelink2012>.
We should be more strict with the range of passage size that we accept but
that's outside of the scope of this PR. I'll merge soon.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#75306 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABNGVTKF64LSINRGAMOLJKTTZV7GJANCNFSM5AJI2DNA>
.
|
|
Ok, thanks. Although Kibana shouldn't rely on the passage size to highlight the text entirely. I'll investigate. |
|
@thelink2012 please enable the option "Allow edits and access to secrets by maintainers" on your PR. For more information, see the documentation. |
thelink2012
force-pushed
the
denilson/improved-highlight-2
branch
from
e263519
to
cafdece
Compare
|
Pinging @elastic/es-search (Team:Search) |
|
As explained here it seems that a mistake happened when testing this patch in Kibana. We were testing the old version (that was reverted), not the fixed one (this PR). I think this PR can be merged. I also ran the Kibana functional tests on master and it passes. |
|
I see there's this new |
|
Hi @mark-vieira. Hope you are doing well! Since you switched version labels, i wonder whether you could help me move this PR forward? Perhaps the PR needs an assignee (which was |
|
Could someone from @elastic/es-search determine if this pull request is still relevant? |
|
Hey @thelink2012, thanks for pushing this. I'm trying to get up to speed on what this is hoping to fix, and I notice that the first of the tests included in the PR, |
|
@elasticmachine generate changelog |
|
Ah, never mind, I've read the attached issue and see what we're trying to do here. All looks good, let's get it up to date and merged. |
|
@elasticmachine update branch |
|
Can you enable the option "Allow edits and access to secrets by maintainers" for me? Our @elasticmachine bot can automate all sorts of stuff for us, but it needs edit access. For more information, see the documentation. |
|
Hi @romseygeek. Thanks for looking at this!! Unfortunately Allow edits and access to secrets by maintainers doesn't work in organization repositories. I have opened this PR again at #89041 in my personal account such that the option is enabled. Please check if that one works :-) References: |
See #73569 for the issue this tries to solve. See PR #73785 for an explanation of the algorithm.
The previous PR had to be reverted due to it causing tests to fail in Kibana (see elastic/kibana#104466).
One of such failures was caused by
targetEndOffsetbeing aintbut having a possibility of storing something bigger than that (more precisely for the Kibana tests,if (targetEndOffset + 1 > textEndIndex)overflowed). Commit e2635198d023fa0ef4b4c7132909d4da19e45574 in this PR fixes that.There were still some failing tests after that. Needs investigation.