Add limits for ngram and shingle settings

[mayya-sharipova]

Create index-level settings:
max_ngram_diff - maximum allowed difference between max_gram and min_gram in
NGramTokenFilter/NGramTokenizer. Default is 1.
max_shingle_diff - maximum allowed difference between max_shingle_size and
min_shingle_size in ShingleTokenFilter. Default is 3.

Throw an IllegalArgumentException when
trying to create NGramTokenFilter, NGramTokenizer, ShingleTokenFilter
where difference between max_size and min_size exceeds the settings value.

Closes #25887

[mayya-sharipova]

retest this please

[mayya-sharipova]

retest this please

[nik9000]

@mayya-sharipova, can you drop the PR template when you open the PR? The checklist is great to go over but not super useful to leave in the description.

[jimczi]

The changes look good @mayya-sharipova
Can you add a small note in the documentation regarding these limits ?
For the shingle for instance it would be in: docs/reference/analysis/tokenfilters/shingle-tokenfilter.asciidoc`

[jimczi]

Maybe use LuceneTestCase#expectThrows

[mayya-sharipova]

I was trying to use LuceneTestCase#expectThrows, but it requires that a constructorpublic NGramTokenizerFactory(..) throw a checked exception. We can add this to the constructor, but then we need to modify all other classes that use NGramTokenizerFactory to catch this exception

[jimczi]

It works fine with a runtime exception, you don't need to change the constructor:

IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () ->
                new NGramTokenizerFactory(indexProperties, null, name, settings).create());

.. and then you can check the message of the exception.

[mayya-sharipova]

thanks Jim, will try this

[jimczi]

Can you add output_unigrams in the formula:
maxGram - minGram + (outputUnigram ? 1 : 0) ?
This way the default diff is 1 since output_unigrams defaults to true.

[jimczi]

Yes too many tokens is problematic. I also don't like that we build invalid boolean queries when the diff is greater than 0. For instance with unigram set to true and shingles of size 2, the input foo bar foobar is analyzed as:
position 1: (foo_bar, foo), position 2: (bar, bar_foobar) ... which is problematic for any position query.
3 seems reasonable especially for fields that don't index positions.

[mayya-sharipova]

@jimczi thanks for your comment. I don't think I understand how diff in shingle size greater that 0 makes boolean queries invalid. Does it mean that if we have tokens: {"token" : "foo_bar", "position" : 1}, {"token" : "foo", "position" : 1}, {"token" : "bar", "position" : 2}, {"token" : "bar_foobar", "position" : 2},, we can't have a phrase query say: "foo bar"?

[jimczi]

foo bar would return the correct document but it would build an invalid phrase query:

"(foo_bar foo) bar"

... trying to find document with foo_bar bar as a phrase query which could be simplified in foo_bar. For boolean query it would not consider that foo_bar is enough to match foo AND bar so the bigram would be useless for matching this type of query. We have solutions on the query side to build the correct query (foo_bar OR (foo AND bar)) but this is currently deactivated because it can explode the number of clauses in the query if you have a large diff between min and max.
For simplicity I prefer when a single shingle size is used for a shingle field. In this case a phrase query like "foo bar foobar baz" could be optimized to "foo_bar foobar_baz" with shingles of size 2 (bar_foobar can be safely ignored since we know that the previous match can only be foo_bar).

[jimczi]

I just realized that this would make the upgrade experience really difficult for indices that break this limit already. They would have to close their index before the upgrade, change the limit in the new version and open the index again. That's a bit too much I believe so to be safe we should check this setting only for indices created before it existed:

if (indexSettings.getIndexVersionCreated().onOrAfter(Version.V_7_0_0_alpha1))

... and only log a deprecation warning if the filter breaks the limit on an index that was created an a previous version (see DeprecationLogger and its usage to see how to do that).
This way we could also backport this new setting in 6.x and makes the upgrade easier.

[mayya-sharipova]

@jimczi Jim, thank you for your feedback. I have addressed your comments in the commit. Could you please review when you have time.

[jimczi]

LGTM, thanks @mayya-sharipova