Fix InternalCategorizationAggregationTests.testReduceRandom #135533
Conversation
elasticsearchmachine
added
needs:triage
| randomIntBetween(10, 100), | ||
| randomLongBetween(1, 10), | ||
| randomIntBetween(1, 100), | ||
| randomIntBetween(50, 100), |
There was a problem hiding this comment.
The failing test had a low similarity threshold and two categories that had a matching 3-letter word. This caused these categories to be merged, leading to unexpected results.
With a higher threshold (which is more realistic anyway), this should not happen anymore.
There was a problem hiding this comment.
Does this mean that the test expects that no categories should ever be merged? If so, this assumption makes the test a little weak, since if none of the aggregations that are being reduced actually do anything, we can't really tell if they're being reduced correctly. For example, I could imagine a bug where instead of correctly combining the aggregators, each just overwrites the previous one, and this test would not be able to detect that, because all of the aggregators have the same behaviour of doing nothing.
It might be difficult to produce a deterministic test case where some of the aggregations actually do combine categories without needing to effectively reimplement the aggregation logic in the test, but as it stands, I'm not sure if this test is actually providing much meaningful coverage.
There was a problem hiding this comment.
All of that is tested in the different other tests in this package.
This test doesn't provide much coverage for categorize text anyway, because random strings generally lead to categories with one string each.
The problem is that the categorize text agg is special, in the sense that it merges / changes keys in the reduce step. No other agg does that (afaik), and the framework isn't compatible with it. In this way, at least some basic properties are verified.
jan-elastic
added
:ml
|
Pinging @elastic/ml-core (Team:ML) |
* upstream/main: (22 commits) Fix InternalCategorizationAggregationTests.testReduceRandom (elastic#135533) [DOCS] GeoIP processor: add clarification about using a reverse proxy endpoint (elastic#135534) Move `ProjectRoutingInfo` and related classes (elastic#135586) Refactor IndexAbstractionResolver (elastic#135587) Simplify returnLocalAll handling in ES|QL (elastic#135353) Reapply "Add an option to return early from an allocate call" (elastic#135589) Mute org.elasticsearch.xpack.esql.qa.single_node.GenerativeIT test elastic#134407 Mute org.elasticsearch.xpack.esql.heap_attack.HeapAttackIT testAggTooManyMvLongs elastic#135585 Mute org.elasticsearch.multiproject.test.XpackWithMultipleProjectsClientYamlTestSuiteIT test {yaml=esql/60_usage/Basic ESQL usage output (telemetry) snapshot version} elastic#135579 Mute org.elasticsearch.search.ccs.KnnVectorQueryBuilderCrossClusterSearchIT testKnnQueryWithCcsMinimizeRoundTripsFalse elastic#135573 Mute org.elasticsearch.xpack.esql.inference.textembedding.TextEmbeddingOperatorTests testSimpleCircuitBreaking elastic#135569 Add telemetry for `TS` command (elastic#135471) Mute org.elasticsearch.cluster.routing.allocation.decider.RestoreInProgressAllocationDeciderTests testCanAllocatePrimaryExistingInRestoreInProgress elastic#135566 allocation: clarify RestoreInProgressAllocationDecider failure message (elastic#132307) [ES|QL] Register AggregateMetricDoubleLiteral (elastic#135054) Validate Logstash pipeline ID when creating. (elastic#135378) Migrate transport versions 8841050 through 8841041 (elastic#135555) Mute org.elasticsearch.search.ccs.SparseVectorQueryBuilderCrossClusterSearchIT testSparseVectorQueryWithCcsMinimizeRoundTripsFalse elastic#135559 Mute org.elasticsearch.action.admin.cluster.stats.SearchUsageStatsTests testToXContent elastic#135558 Testing indices query cache memory stats (elastic#135298) ...
fixes: #87240