-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Avoid unnecessary determinization in index pattern conflict checks #128362
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
base: main
Are you sure you want to change the base?
Conversation
Starting with Lucene 10, `CharacterRunAutomaton` is no longer determinized automatically. In Elasticsearch 9, we adapted to this by eagerly determinizing automatons early (via `Regex#simpleMatchToAutomaton`). However, this introduced regression: operations like index template conflict checks, which only require intersection testing, now pay the cost of determinization—an expensive step that wasn’t needed before. In some cases, especially when many wildcard patterns are involved, determinization can even fail due to state explosion. This change removes the unnecessary determinization, restoring the pre-9.0 behavior and allowing valid index templates with many patterns to be registered again.
Pinging @elastic/es-data-management (Team:Data Management) |
Hi @jimczi, I've created a changelog YAML for you. |
/** | ||
* Return a deterministic Automaton that matches the union of the provided patterns. | ||
*/ | ||
public static Automaton simpleMatchToAutomaton(String... patterns) { |
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.
small drive-by comment: should add to the javadocs when one or the other should be used?
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.
+1, Luca. I've raised a similar question internally within the relevant discussion thread.
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.
++, 268dc56
I also added the variant for the single pattern case since to be complete.
…ndex_patterns_automaton
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.
LGTM overall. Leaving 2 tiny comments.
Automaton b = Regex.simpleMatchToNonDeterminizedAutomaton(Arrays.copyOfRange(patterns, patterns.length / 2, patterns.length)); | ||
assertFalse(Operations.isEmpty(Operations.intersection(a, b))); | ||
IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> assertMatchesAll(a, "my_test")); | ||
assertThat(exc.getMessage(), containsString("deterministic")); |
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.
Nit pick: can we improve the expectation around the exception's error message? Readers like me could be curious about what kind of message to expect.
@@ -250,4 +254,14 @@ public void testThousandsAndLongPattern() throws IOException { | |||
assertTrue(predicate.test(patterns[i])); | |||
} | |||
} | |||
|
|||
public void testIntersectNonDeterminizedAutomaton() { | |||
String[] patterns = randomArray(20, 100, size -> new String[size], () -> "*" + randomAlphanumericOfLength(10) + "*"); |
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.
Could we add some minor detailing about how these size-related values were chosen? If I'm not mistaken, don't they relate to the default determinisation limit that comes from Lucene?
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.
LGTM, thanks for tagging us
Starting with Lucene 10,
CharacterRunAutomaton
is no longer determinized automatically.In Elasticsearch 9, we adapted to this by eagerly determinizing automatons early (via
Regex#simpleMatchToAutomaton
). However, this introduced regression: operations like index template conflict checks, which only require intersection testing, now pay the cost of determinization, an expensive step that wasn’t needed before. In some cases, especially when many wildcard patterns are involved, determinization can even fail due to state explosion.This change removes the unnecessary determinization for index patterns conflict check, restoring the pre-9.0 behavior and allowing valid index templates with many patterns to be registered again.
closes: #127972