feat(fts): add ngram tokenizer - #593
Open
egolearner wants to merge 3 commits into
Open
Conversation
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| #include <string> |
| const auto &token_chars = token_chars_val.as_array(); | ||
| for (const auto &token_char_val : token_chars) { | ||
| if (!token_char_val.is_string()) { | ||
| LOG_ERROR("NGramTokenizer: token_chars entries must be strings"); |
Collaborator
There was a problem hiding this comment.
这里要不还是返回status，这样用户create collection或者create fts index能通过返回拿到错误信息？
| * emits UTF-8 codepoint ngrams from each span. Consecutive CJK text remains in | ||
| * the same base span so CJK ngrams can be generated. | ||
| */ | ||
| class NGramTokenizer : public Tokenizer { |
Collaborator
There was a problem hiding this comment.
为啥不挪到单独的一个头文件 ngram_tokenizer.h?
Collaborator
Author
There was a problem hiding this comment.
ngram/standard有些共用逻辑，已重构加上unicode_util.h/cc，然后分开ngram/standard为不同的文件
| } | ||
|
|
||
| bool NGramTokenizer::init(const ailego::JsonObject &config) { | ||
| if (!parse_positive_uint32_param(config, "ngram_min", kDefaultNGramLength, |
Collaborator
There was a problem hiding this comment.
minor：遇到不认识的参数可以抛个warning
Collaborator
Author
There was a problem hiding this comment.
先不加了吧，由于json采用打平的方式，加这个逻辑会比较重。
| if (configured_value <= 0 || | ||
| configured_value > | ||
| static_cast<int64_t>(std::numeric_limits<uint32_t>::max())) { | ||
| LOG_ERROR("NGramTokenizer: %s must be positive uint32", key); |
Collaborator
There was a problem hiding this comment.
建议增加 NGram 数量膨胀保护。目前只校验 ngram_min <= ngram_max，但没有限制两者差值。由于实现会将所有 token 及其 substr() 结果一次性存入 vector，例如长度为 10,000 的文本配置 min=1、max=10000 时，会产生约 5,000 万个 token，存在严重的 CPU 和内存风险。
建议参考 Elasticsearch 默认的 max_ngram_diff=1 增加差值限制，并在运行时增加 token 数量或总字节数预算，同时补充相关边界测试。
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No description provided.