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
Term positions in analyze API should start at 1, not 0 #10771
Term positions in analyze API should start at 1, not 0 #10771
Conversation
LGTM |
@johtani Let's make this change only master - it's a breaking change. |
@clintongormley I see. |
int increment = posIncr.getPositionIncrement(); | ||
if (increment > 0) { | ||
position = position + increment; | ||
} | ||
tokens.add(new AnalyzeResponse.AnalyzeToken(term.toString(), position, offset.startOffset(), offset.endOffset(), type.type())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look correct to me: of course
would be analyzer as a single token course
at position 0 while it should be at position 1.
I think the right fix is to put back tokens.add
where it was before and instead to initialize position
at -1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks!
@jpountz Fix your comment and add the test . |
LGTM. Thanks @johtani ! |
f8efba7
to
ca3d941
Compare
ca3d941
to
933edf7
Compare
Now, Analyze API return 1 as 1st token.
Analyze API should return 0 as 1st token, looks like TermVector.