Skip to content

Add radix merge logic to CTOptimizedTrie deletion#12

Merged
jordanmontt merged 5 commits intopharo-containers:masterfrom
HossamSaberr:master
Mar 10, 2026
Merged

Add radix merge logic to CTOptimizedTrie deletion#12
jordanmontt merged 5 commits intopharo-containers:masterfrom
HossamSaberr:master

Conversation

@HossamSaberr
Copy link
Copy Markdown
Contributor

As discussed in issue #11, this PR adds a reproducible test case in CTOptimizedTrieTest.

Currently, the test fails because CTOptimizedTrie does not merge a parent and child node when a deletion leaves an intermediate node with only one child. This confirms the structural degradation and memory bloat.

@HossamSaberr
Copy link
Copy Markdown
Contributor Author

Hey @jordanmontt, I've just pushed the fix to this same PR.
and it Green now

image

It overrides compressNode:ancestors: in CTOptimizedTrie to securely merge an intermediate node with its single child when a deletion leaves it redundant. This restores the Radix tree structure and makes the test pass locally. Let me know what you think.

@HossamSaberr HossamSaberr changed the title Add failing test for CTOptimizedTrie on deletion (#11) Add radix merge logic to CTOptimizedTrie deletion Feb 24, 2026

trie removeKey: 'cat'.

self assert: (trie at: 'car' ifAbsent: [ nil ]) equals: 1.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not use ifabsent in this case

self assert: (trie at: 'car') equals: 1.


{ #category : #'private - accessing' }
{ #category : 'removing' }
CTOptimizedTrie >> compressNode: aNode ancestors: aCollection [
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some tests to test this method #compressNode:ancestors:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. I'm working on these <3

@jordanmontt
Copy link
Copy Markdown
Member

Good PR! It just needs some more tests. I left a review

@HossamSaberr
Copy link
Copy Markdown
Contributor Author

Updates pushed. Added tests and fixed the assertion.

@jordanmontt
Copy link
Copy Markdown
Member

There is still no test that tests directly the compressNode:ancestors: method. We need some unit tests that test only that method and calling it directly :)

@HossamSaberr
Copy link
Copy Markdown
Contributor Author

Done, Let me know if this looks good!

@jordanmontt jordanmontt merged commit 4c3622c into pharo-containers:master Mar 10, 2026
4 checks passed
@github-actions
Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 22639616922

Details

  • 19 of 19 (100.0%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.6%) to 80.0%

Files with Coverage Reduction New Missed Lines %
src/Containers-Trie/CTOptimizedTrieNode.class.st 5 84.17%
Totals Coverage Status
Change from base Build 22344752276: 0.6%
Covered Lines: 376
Relevant Lines: 470

💛 - Coveralls

@coveralls
Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 22639616922

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 19 of 19 (100.0%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.6%) to 80.0%

Files with Coverage Reduction New Missed Lines %
src/Containers-Trie/CTOptimizedTrieNode.class.st 5 84.17%
Totals Coverage Status
Change from base Build 22344752276: 0.6%
Covered Lines: 376
Relevant Lines: 470

💛 - Coveralls

1 similar comment
@coveralls
Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 22639616922

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 19 of 19 (100.0%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.6%) to 80.0%

Files with Coverage Reduction New Missed Lines %
src/Containers-Trie/CTOptimizedTrieNode.class.st 5 84.17%
Totals Coverage Status
Change from base Build 22344752276: 0.6%
Covered Lines: 376
Relevant Lines: 470

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants