-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Remove MinimizationOperations #17268
Remove MinimizationOperations #17268
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17268 +/- ##
============================================
- Coverage 72.40% 72.39% -0.01%
+ Complexity 65554 65488 -66
============================================
Files 5292 5291 -1
Lines 304493 304323 -170
Branches 44218 44176 -42
============================================
- Hits 220463 220316 -147
+ Misses 65975 65951 -24
- Partials 18055 18056 +1 ☔ View full report in Codecov by Sentry. |
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.
Thanks for removing this: it is a killer.
A lot of these will just come out minimal-from-the-getgo with lucene, but minimizing is never necessary, and its very costly. Just look at how terrifying the code you deleted is :)
Thanks @msfroh , this one is solely on me: I brought it into 3.x line to unlock the Lucene 10 migration, thanks for cleaning things up! |
❌ Gradle check result for 0eb2b5a: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
This class was removed from Lucene because it is no longer needed. It was copied into the OpenSearch code base because we were using it and we didn't want our code to break. In fact, the correct choice would have been to remove the references to `MinimizationOperations.minimize()` and either replace them with calls to `Operations.determinize()` or simply drop them altogether because the automaton is deterministic. Signed-off-by: Michael Froh <[email protected]>
Signed-off-by: Michael Froh <[email protected]>
0eb2b5a
to
bad6e8b
Compare
Description
This class was removed from Lucene because it is no longer needed. It was copied into the OpenSearch code base because we were using it and we didn't want our code to break.
In fact, the correct choice would have been to remove the references to
MinimizationOperations.minimize()
and either replace them with calls toOperations.determinize()
or simply drop them altogether because the automaton is deterministic.Related Issues
Resolves #16975
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.