refactor(merge-tree): drop unnecessary red-black tree usage - #28128
Open
Matt Rakow (ChumpChief) wants to merge 3 commits into
Open
refactor(merge-tree): drop unnecessary red-black tree usage#28128Matt Rakow (ChumpChief) wants to merge 3 commits into
Matt Rakow (ChumpChief) wants to merge 3 commits into
Conversation
…kups Client.clientNameToIds and TestServer.upstreamMap used RedBlackTree purely as a key/value dictionary - neither called any ordered operation (floor, ceil, min, max, walk, mapRange). A plain Map provides the same behavior with O(1) lookup instead of O(log n) pointer chasing. Note that getOrAddShortClientId previously tested the returned node for truthiness, which was only correct because a node object is always truthy. The Map equivalent uses has() so that short client id 0 is handled correctly. This is a step toward removing the hand-rolled red-black tree implementation entirely. All types involved are internal-only, so there is no API surface change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`simpleTest`, `integerTest1` and `fileTest1` are exported from beastTest.spec.ts but nothing calls them: the suite only runs `firstTest`, `randolicious`, `mergeTreeCheckedTest`, `clientServer` and `findReplacePerf`, none of which touch a RedBlackTree. Removing them also retires `LinearDictionary`, a hand-written array-backed SortedDictionary which existed solely as the oracle `fileTest1` compared the tree against, along with the two property printers and `took`. No test which runs today loses any coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
area: tools
area: dds
Contributor
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (264 lines, 4 files), I've queued these reviewers:
How this works
|
Contributor
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`compareNumbers` and `compareStrings` in mergeTreeNodes.ts have no remaining callers. They were removed from the public API in microsoft#17952 after a deprecation period, are not re-exported from any barrel and appear in no api-report, and the last references were the RedBlackTree instantiations and LinearDictionary comparator removed earlier in this PR. The identically named helpers in `dds/tree` and `id-compressor` are separate local definitions and are unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Matt Rakow (ChumpChief)
requested review from
Abram Sanderson (Abe27342) and
Tony Murphy (anthony-murphy)
August 31, 2026 17:09
Contributor
Fleet Review — In progressRunning reviewers: correctness, security, api-compatibility, performance |
shlevari
approved these changes
Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
merge-treeships a hand-written red-black tree (src/collections/rbTree.ts, ~765 lines) which I'd like to remove. This PR is the first, smallest step: it clears out the two uses that aMaphandles, and deletes some dead test code that mentions the tree. It does not touch any of the ordered-lookup consumers, and it does not deleterbTree.ts— those follow in separate PRs.#27966 has the complete removal, including measurements for every replacement. It's being split into reviewable pieces, of which this is the first; read it there if you want the full picture before reviewing the parts.
Replace
RedBlackTreewithMap.Client.clientNameToIdsandTestServer.upstreamMapare plain key/value lookups. Neither ever iterates in order, walks a range, or asks for a nearest neighbour — the only operations are get, put, and existence checks. A red-black tree buys nothing here beyond a comparator and an allocation per entry.This also fixes a latent bug in
getOrAddShortClientId, which tested membership withif (!this.clientNameToIds.get(id)). That is only correct becauseRedBlackTree.getreturns anRBNodewrapper, which is always truthy — the moment the container changed to one returning the stored value, a legitimately falsy value would have been treated as absent. TheMapversion uses.has().Remove dead test code.
beastTest.spec.tsexportssimpleTest,integerTest1andfileTest1, none of which are called by anything: noit()ordescribe()wraps them, and there are no references anywhere else in the repo. Removing them also retiresLinearDictionary, a hand-written array-backedSortedDictionarythat existed solely as the oraclefileTest1compared the tree against, plus two property printers and atooktiming helper. These were the only mentions ofRedBlackTreein the test suite, but since none of them run, no test loses coverage.Delete the orphaned comparators. With those gone,
compareNumbersandcompareStringsinmergeTreeNodes.tshave no callers left — they had been feeding theRedBlackTreeinstantiations andLinearDictionary. Both were removed from the public API in #17952 after a deprecation period, are not re-exported from any barrel, and appear in no api-report. The identically named helpers indds/treeandid-compressorare separate local definitions and are unaffected.No public API is affected — every type involved is
@internaland none appear in an api-report.Tracked by AB#80505.
Reviewer Guidance
The review process is outlined in the pull request guidelines.
The three commits are best read in order — each removal is what strands the next.
This is deliberately the boring slice of a larger cleanup, split out so the interesting parts can be reviewed on their own. Still to come, as separate PRs carved out of #27966, in rough order:
endpointIndex,endpointInRangeIndex,startpointInRangeIndex) with a sharedSortedSetsubclass. Includes a bug fix: intervals sharing an end position currently overwrite one another.OverlappingIntervalsIndexwith a sorted array plus a segment tree of maximum end positions.rbTree.tsand its exports.