[ML] Include out-of-order as well as in-order terms in reverse search #950
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When categories include messages with different token orders, we were not including the tokens whose order varied between the messages in the terms of the reverse search. This change adds these out-of-order tokens that are part of the category definition back into the reverse search terms. Fixes the easy part of elastic#949
droberts195
commented
Jan 17, 2020
| // reverse search creation are tested more thoroughly in the unit tests for | ||
| // their respective classes, but this test helps to confirm that they work | ||
| // together) | ||
| BOOST_TEST_REQUIRE(output.find("\"terms\":\"Killing job\"") != std::string::npos); |
There was a problem hiding this comment.
The fix of this PR makes terms here Killing job where before this PR it would have been empty.
| // their respective classes, but this test helps to confirm that they work | ||
| // together) | ||
| BOOST_TEST_REQUIRE(output.find("\"terms\":\"Killing job\"") != std::string::npos); | ||
| BOOST_TEST_REQUIRE(output.find("\"regex\":\".*\"") != std::string::npos); |
There was a problem hiding this comment.
The regex is still poor, .*, after this PR. A followup that will not be backported as far will fix this.
See #949 (comment) for details.
benwtrent
approved these changes
Jan 17, 2020
Yes, we will if some aspect of the category changes as a result of assigning a new message to it. But it will only make the category definitions stored in the results index better, so I don't think it's a problem. The category definition stored in the opaque categorizer state that the C++ uses is not changed at all by this fix. The problem being fixed here is that the way that C++ internal state was reported in a more friendly format was losing some information in this edge case. The other part of the fix to #949 will involve changing the C++ internal state, and that is why it is too difficult/dangerous to include in a 6.8 patch. |
tveasey
approved these changes
Jan 20, 2020
| size_t end(category.outOfOrderCommonTokenIndex()); | ||
| for (size_t index = 0; index < end; ++index) { | ||
| size_t tokenId(baseTokenIds[index].first); | ||
| bool first{true}; |
There was a problem hiding this comment.
I'm not sure about this name. Maybe firstInOrderToken would be clearer?
droberts195
added a commit
to droberts195/ml-cpp
that referenced
this pull request
Jan 20, 2020
…earch When categories include messages with different token orders, we were not including the tokens whose order varied between the messages in the terms of the reverse search. This change adds these out-of-order tokens that are part of the category definition back into the reverse search terms. Backport of elastic#950
droberts195
added a commit
to droberts195/ml-cpp
that referenced
this pull request
Jan 20, 2020
…earch When categories include messages with different token orders, we were not including the tokens whose order varied between the messages in the terms of the reverse search. This change adds these out-of-order tokens that are part of the category definition back into the reverse search terms. Backport of elastic#950
This was referenced Jan 20, 2020
droberts195
added a commit
to droberts195/ml-cpp
that referenced
this pull request
Jan 20, 2020
…earch When categories include messages with different token orders, we were not including the tokens whose order varied between the messages in the terms of the reverse search. This change adds these out-of-order tokens that are part of the category definition back into the reverse search terms. Backport of elastic#950
droberts195
added a commit
that referenced
this pull request
Jan 20, 2020
…earch (#954) When categories include messages with different token orders, we were not including the tokens whose order varied between the messages in the terms of the reverse search. This change adds these out-of-order tokens that are part of the category definition back into the reverse search terms. Backport of #950
droberts195
added a commit
that referenced
this pull request
Jan 20, 2020
…earch (#956) When categories include messages with different token orders, we were not including the tokens whose order varied between the messages in the terms of the reverse search. This change adds these out-of-order tokens that are part of the category definition back into the reverse search terms. Backport of #950
droberts195
added a commit
that referenced
this pull request
Jan 20, 2020
…earch (#955) When categories include messages with different token orders, we were not including the tokens whose order varied between the messages in the terms of the reverse search. This change adds these out-of-order tokens that are part of the category definition back into the reverse search terms. Backport of #950
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When categories include messages with different token orders,
we were not including the tokens whose order varied between
the messages in the terms of the reverse search. This change
adds these out-of-order tokens that are part of the category
definition back into the reverse search terms.
Fixes the easy part of #949