test(search): make ranking tokens collision-free and stop swallowing poll errors (1.13) - #30251
Conversation
…poll errors SearchEntityRankingIT's "Tier1 > untagged on a text tie" case seeds two documents with an identical displayName so their text scores are equal, then asserts the Tier.Tier1 global term boost breaks the tie. The tie was not actually exact. RankingSupport.uniqueTerm() derived its token from TestNamespace.uniqueShortId(), which is RUN_ID.substring(0,8) + methodHash + 4 random chars — only 4 of 16 characters vary within a test method. Every token minted for one test therefore shared a 14-character prefix, and EntitySeeder.nameFor() places a sibling token in each entity's name, so the query ngram-matched both documents' names by differing amounts. That leaked a variable name.ngram / displayName.ngram contribution into scores the case requires to be equal. Measured against 1.13 (Tier1 boost 0.05, multiplier 1.05x): 3/25 inversions. The noise ratio is bounded by roughly 1.17 because the ngram terms sit in the "max plus 0.3 times others" bucket while displayName.keyword dominates, so main's post-#29903 boost of 0.5 (1.5x) sits above the noise and never inverts — the defect is present on main too, just out of range. Drawing the token from its own full-entropy source removes the overlap at source rather than relying on a boost margin to outweigh it. Also stop reporting every awaitTrue timeout as "not indexed in time". ignoreExceptions() is correct while polling a document that is still being indexed, but discarding the exception made a condition that threw on every poll indistinguishable from genuine indexing lag. awaitOrReason() keeps the tolerance and reports the last error alongside the timeout. RankingSupportTest covers both collision properties; it fails with a 14-char shared prefix against the previous implementation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
backend
safe to test
Code Review ✅ ApprovedEliminates token collision in the search ranking test suite by increasing entropy and improves error visibility in OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -0,0 +1,65 @@ | |||
| package org.openmetadata.it.tests.search; | |||
There was a problem hiding this comment.
This new Java file has no Apache 2.0 license header. The repository requires that header on every new source file, so the license check can reject the build before these tests run.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| for (int i = 0; i < terms.size(); i++) { | ||
| for (int j = i + 1; j < terms.size(); j++) { | ||
| int shared = sharedPrefixLength(terms.get(i), terms.get(j)); | ||
| assertTrue( |
There was a problem hiding this comment.
This assertion can fail for valid UUID output when two sampled terms happen to share seven random characters after the fixed zz prefix. The implementation would still be collision-free, but this test would fail nondeterministically; use deterministic token inputs or test a guaranteed structural property instead.
| private static String describeTimeout(Exception lastError) { | ||
| String reason = "not satisfied within " + INDEX_WAIT; | ||
| if (lastError != null) { | ||
| reason = reason + "; last error: " + lastError; |
There was a problem hiding this comment.
lastError is normally the IllegalStateException created by search(), whose message only identifies the index and query. Appending its toString() omits the nested HTTP or deserialization cause, so a persistent 4xx or malformed response still times out without showing the detail needed to distinguish it from indexing lag.
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
🔴 Playwright Results — 2 pipeline/setup failure(s)✅ 3957 passed · ❌ 0 failed · 🟡 22 flaky · ⏭️ 30 skipped Pipeline and setup failures
🟡 22 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
2 participants
Describe your changes:
Full fix for the
SearchEntityRankingITranking failures on1.13. Keeps both parts: the collision-free token change and theawaitOrReasonpoll-error surfacing.1.13 is where this actually bites. Main is green today only because its post-#29903 Tier1 boost of
0.5sits above the noise ceiling; 1.13 ships0.05, which sits inside it, so the tie case inverts at a measured ~12% rate per entity type. This is the fix for theSearchEntityRankingITranking failures on the 1.13 CI runs.SearchEntityRankingIT's "Tier1 > untagged on a text tie" case seeds two documents with an identicaldisplayNameso their text scores are equal, then asserts theTier.Tier1global term boost breaks the tie. The tie was never actually exact.RankingSupport.uniqueTerm()derived its token fromTestNamespace.uniqueShortId(), which isRUN_ID.substring(0,8)+methodHash+ 4 random chars — only 4 of 16 characters vary within a test method. Every token minted for one test therefore shared a 14-character prefix, andEntitySeeder.nameFor()places a sibling token in each entity's name, so the query ngram-matched both documents' names by differing amounts. That leaked a variablename.ngram/displayName.ngramcontribution into scores the case requires to be equal.The signature is visible in the failing output — three documents in a result set where the case seeded two:
explain=trueconfirms the text-match components are byte-identical on both documents (displayName.keyword21.5011,displayName8.0629) — the entire delta comes from the ngram subqueries.Why main is green today and 1.13 is not. The noise ratio is bounded around 1.17, because the ngram terms sit in the
max plus 0.3 times othersbucket whiledisplayName.keyworddominates at boost 26.4. Main's post-#29903 Tier1 boost of0.5(1.5x) sits above that ceiling; 1.13 ships0.05(1.05x), which sits inside it. The defect is present on main too — just out of range. Measured on a live 1.13.1 server, varying only the boost:0.05(1.13 ships)0.5(main ships, post-#29903)Drawing the token from its own full-entropy source removes the overlap at source rather than relying on a boost margin to outweigh it.
Also: stop swallowing poll errors.
awaitTruereported every timeout as a flat "not indexed in time".ignoreExceptions()is correct while polling a document that is still being indexed, but discarding the exception made a condition that threw on every poll indistinguishable from genuine indexing lag.awaitOrReason()keeps the tolerance and reports the last error alongside the timeout, so real failures identify themselves instead of hiding behind a generic message.How I tested
RankingSupportTestcovers both collision properties (hex-free, and no long shared prefix between sibling tokens). Negative control: patcheduniqueTermback to the old scheme and the test fails withshare 14 characters— matching the CI signature exactly.0.05boost: 3/25 inversions before the fix, 0/25 after. The fix stands on its own and does not depend on backporting Harden search ranking relevance #29903.Type of change:
Checklist:
Greptile Summary
This PR removes ranking-token overlap and improves search polling diagnostics. The main changes are:
Confidence Score: 4/5
The token changes look sound, but the new test file can fail repository checks and contains a nondeterministic assertion.
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/search/RankingSupportTest.java and openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/search/RankingSupport.java
Important Files Changed
Reviews (1): Last reviewed commit: "test(search): make ranking tokens collis..." | Re-trigger Greptile
Context used: