Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent DirectCandidateGenerator to reuse an unclosed analyzer #12670

Merged
merged 2 commits into from
Aug 5, 2015

Conversation

nomoa
Copy link
Contributor

@nomoa nomoa commented Aug 5, 2015

When postFilter generates a token that is identical to the input term
DirectCandidateGenerator should not preFilter this token. If postFilter
and preFilter are the same analyzer instance it would fail with :
"TokenStream contract violation: close() call missing"

When postFilter generates a token that is identical to the input term
DirectCandidateGenerator should not preFilter this token. If postFilter
and preFilter are the same analyzer instance it would fail with :
"TokenStream contract violation: close() call missing"
@nik9000
Copy link
Member

nik9000 commented Aug 5, 2015

Hi @nomoa! You'll have to sign the CLA before I can merge this but I'll review it now.

@s1monw
Copy link
Contributor

s1monw commented Aug 5, 2015

hey, can you maybe sign the CLA so we can merge it?

@nik9000 nik9000 added the :Search Relevance/Suggesters "Did you mean" and suggestions as you type label Aug 5, 2015
@nomoa
Copy link
Contributor Author

nomoa commented Aug 5, 2015

Signed, it's weird because I already signed it last time... I will re-sign :)
The first time I used "nomoa" as github username.
This time I used my email address.

@nomoa
Copy link
Contributor Author

nomoa commented Aug 5, 2015

OK signed again with the correct email address :)

@@ -152,7 +152,7 @@ public void nextToken() throws IOException {

if (posIncAttr.getPositionIncrement() > 0 && result.get().bytesEquals(candidate.term)) {
BytesRef term = result.toBytesRef();
long freq = frequency(term);
long freq = internalFrequency(term);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a comment on why internalFrequency is the right thing to do here.

@s1monw
Copy link
Contributor

s1monw commented Aug 5, 2015

LGTM I think this is because of synonym handling maybe? I with I remember / would have documented it :(

@nik9000 nik9000 self-assigned this Aug 5, 2015
@nik9000
Copy link
Member

nik9000 commented Aug 5, 2015

LGTM. Thanks for adding the comments.

@nik9000
Copy link
Member

nik9000 commented Aug 5, 2015

I'll do the merging if that is ok with @s1monw .

@s1monw
Copy link
Contributor

s1monw commented Aug 5, 2015

++

nik9000 added a commit that referenced this pull request Aug 5, 2015
Prevent DirectCandidateGenerator to reuse an unclosed analyzer
@nik9000 nik9000 merged commit 7f22fa1 into elastic:1.7 Aug 5, 2015
@nomoa
Copy link
Contributor Author

nomoa commented Aug 5, 2015

Thanks!

@nik9000
Copy link
Member

nik9000 commented Aug 5, 2015

Thanks @nomoa !

nik9000 added a commit that referenced this pull request Aug 5, 2015
When postFilter generates a token that is identical to the input term
DirectCandidateGenerator should not preFilter this token. If postFilter
and preFilter are the same analyzer instance it would fail with :
"TokenStream contract violation: close() call missing"

This is a forward port of @nomoa's #12670
@nik9000
Copy link
Member

nik9000 commented Aug 5, 2015

And I've finally merged this to master. I couldn't figure out a good way to forward port it and keep your name on it, sorry about that. I referenced you in the commit message at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants