From e76fbe304a6c952933a1e43adcee95f03100c31b Mon Sep 17 00:00:00 2001 From: Marcus Denker Date: Wed, 7 Feb 2024 15:29:49 +0100 Subject: [PATCH] This PR moves the key convertion (asIndexKeyOfSize:) to the iterator SoilIndexIterator>>#find:ifAbsent: now does the convertion - This allows us to remove is from #find: and the parametor of find: in #removeKey:ifAbsent: in the index. - #find: is now in sync between the index and the iterator (taking key, not binKey). The PR changes most of the tests in SoilIndexedDictionaryTest to not use integer keys, as this can hide bugs. (there are some not yet changed, the ones for nextAssociation/lastAssociation and testNextKeyCloseToWithTransaction, that will be another PR as there the change seems to show another bug --- .../SoilIndexedDictionaryTest.class.st | 79 +++++++++---------- src/Soil-Core/SoilBTreeIterator.class.st | 4 +- src/Soil-Core/SoilBasicBTree.class.st | 12 --- src/Soil-Core/SoilBasicSkipList.class.st | 12 --- src/Soil-Core/SoilIndex.class.st | 14 +++- src/Soil-Core/SoilIndexIterator.class.st | 13 ++- src/Soil-Core/SoilSkipListIterator.class.st | 4 +- 7 files changed, 57 insertions(+), 81 deletions(-) diff --git a/src/Soil-Core-Tests/SoilIndexedDictionaryTest.class.st b/src/Soil-Core-Tests/SoilIndexedDictionaryTest.class.st index a0c8ad46..b0ff9d3d 100644 --- a/src/Soil-Core-Tests/SoilIndexedDictionaryTest.class.st +++ b/src/Soil-Core-Tests/SoilIndexedDictionaryTest.class.st @@ -134,13 +134,13 @@ SoilIndexedDictionaryTest >> testAtIndexWithTransaction [ | tx tx2 | tx := soil newTransaction. tx root: dict. - dict at: 1 put: #one. - dict at: 2 put: #two. + dict at: #one put: #onevalue. + dict at: #two put: #twovalue. tx commit. "open a second transaction ..." tx2 := soil newTransaction. "and test atIndex:" - self assert: (tx2 root atIndex: 1) equals: #one + self assert: (tx2 root atIndex: 1) equals: #onevalue ] @@ -233,8 +233,8 @@ SoilIndexedDictionaryTest >> testDoWithTransAction [ tx := soil newTransaction. tx root: dict. - dict at: 1 put: #bar1. - dict at: 2 put: #bar2. + dict at: #one put: #bar1. + dict at: #two put: #bar2. tx commit. "open a second transaction ..." tx1 := soil newTransaction. @@ -246,7 +246,7 @@ SoilIndexedDictionaryTest >> testDoWithTransAction [ counter := counter + 1]. self assert: counter equals: 2. - tx2 root removeKey: 1. + tx2 root removeKey: #one. counter := 0. tx2 root do: [ :each | @@ -385,7 +385,7 @@ SoilIndexedDictionaryTest >> testIsEmpty [ | tx tx1 tx2 | tx := soil newTransaction. tx root: dict. - dict at: 1 put: #one. + dict at: #one put: #oneValue. tx commit. "open a second transaction ..." tx1 := soil newTransaction. @@ -393,7 +393,7 @@ SoilIndexedDictionaryTest >> testIsEmpty [ self deny: tx1 root isEmpty. tx2 := soil newTransaction. - tx2 root removeKey: 1. + tx2 root removeKey: #one. self assert: tx2 root size equals: 0. self assert: tx2 root isEmpty. tx2 commit. @@ -443,15 +443,15 @@ SoilIndexedDictionaryTest >> testLastWithTransaction [ | tx tx2 | tx := soil newTransaction. tx root: dict. - dict at: 2 put: #two. - dict at: 1 put: #one. + dict at: #two put: #twovalue. + dict at: #one put: #onevalue. "self assert: dict last equals: #two." tx commit. "open a second transaction ..." tx2 := soil newTransaction. "and test last, note: keyorder" - self assert: tx2 root last equals: #two + self assert: tx2 root last equals: #twovalue ] { #category : #tests } @@ -459,15 +459,15 @@ SoilIndexedDictionaryTest >> testLastWithTransactionRemoveLast [ | tx tx2 | tx := soil newTransaction. tx root: dict. - dict at: 1 put: #one. - dict at: 2 put: #two. + dict at: #one put: #onevalue. + dict at: #two put: #twovalue. "self assert: dict last equals: #two." tx commit. "open a second transaction ..." tx2 := soil newTransaction. - tx2 root removeKey: 2. + tx2 root removeKey: #two. "and test last" - self assert: tx2 root last equals: #one + self assert: tx2 root last equals: #onevalue ] { #category : #tests } @@ -475,14 +475,13 @@ SoilIndexedDictionaryTest >> testNextAssociationAfterWithTransaction [ | tx tx2 | tx := soil newTransaction. tx root: dict. - dict at: 1 put: #one. - dict at: 2 put: #two. - "self assert: dict last equals: #two." + dict at: #one put: #onevalue. + dict at: #two put: #twovalue. tx commit. "open a second transaction ..." tx2 := soil newTransaction. "and test last" - self assert: (tx2 root nextAssociationAfter: 1) value equals: #two + self assert: (tx2 root nextAssociationAfter: #one) value equals: #twovalue ] { #category : #tests } @@ -507,22 +506,21 @@ SoilIndexedDictionaryTest >> testRemoveKeyIfAbsentWithTransaction [ | tx tx2 tag | tx := soil newTransaction. tx root: dict. - dict at: 1 put: #one. - dict at: 2 put: #two. - "self assert: dict last equals: #two." + dict at: #one put: #onevalue. + dict at: #two put: #twovalue. tx commit. "open a second transaction ..." tx2 := soil newTransaction. "remove the key" - tx2 root removeKey: 2. + tx2 root removeKey: #two. self assert: tx2 root size equals: 1. "remove again to test absent case" tag := false. - tx2 root removeKey: 3 ifAbsent: [ tag := true ]. + tx2 root removeKey: #three ifAbsent: [ tag := true ]. self assert: tag. "remove again to test absent case with already removed key" tag := false. - tx2 root removeKey: 2 ifAbsent: [ tag := true ]. + tx2 root removeKey: #two ifAbsent: [ tag := true ]. self assert: tag. @@ -537,19 +535,19 @@ SoilIndexedDictionaryTest >> testRemoveKeyWithTwoTransactions [ self skip. tx := soil newTransaction. tx root: dict. - dict at: 1 put: #one. - dict at: 2 put: #two. + dict at: #one put: #onevalue. + dict at: #two put: #twovalue. tx commit. "we create two transactions" tx := soil newTransaction. tx2 := soil newTransaction. "remove the key" - tx2 root removeKey: 2. + tx2 root removeKey: #two. tx2 commit. "check that we can still see in the first tr" - self assert: (tx root at: 2) equals: #two. + self assert: (tx root at: #two) equals: #twovalue. "but removeKey: does not see it, we can remove it without error" - tx root removeKey: 2 ifAbsent: [ self fail ]. + tx root removeKey: #two ifAbsent: [ self fail ]. self assert: tx root size equals: 1. "but commiting it will fail, as we have commited the remove in t2" self should: [tx commit] raise: SoilObjectHasConcurrentChange @@ -560,14 +558,13 @@ SoilIndexedDictionaryTest >> testSecondWithTransaction [ | tx tx2 | tx := soil newTransaction. tx root: dict. - dict at: 1 put: #one. - dict at: 2 put: #two. - "self assert: dict last equals: #two." + dict at: #one put: #onevalue. + dict at: #two put: #twovalue. tx commit. "open a second transaction ..." tx2 := soil newTransaction. "and test last" - self assert: tx2 root second equals: #two + self assert: tx2 root second equals: #twovalue ] { #category : #tests } @@ -575,9 +572,8 @@ SoilIndexedDictionaryTest >> testSizeWithTransaction [ | tx tx2 | tx := soil newTransaction. tx root: dict. - dict at: 1 put: #one. - dict at: 2 put: #two. - "self assert: dict last equals: #two." + dict at: #one put: #onevalue. + dict at: #two put: #twovalue. tx commit. "open a second transaction ..." tx2 := soil newTransaction. @@ -590,14 +586,13 @@ SoilIndexedDictionaryTest >> testValuesWithTransaction [ | tx tx2 | tx := soil newTransaction. tx root: dict. - dict at: 1 put: #one. - dict at: 2 put: #two. - "self assert: dict last equals: #two." + dict at: #one put: #onevalue. + dict at: #two put: #twovalue. tx commit. "open a second transaction ..." tx2 := soil newTransaction. "and test last" self assert: tx2 root values size equals: 2. - self assert: (tx2 root values includes: #one). - self assert: (tx2 root values includes: #two) + self assert: (tx2 root values includes: #onevalue). + self assert: (tx2 root values includes: #twovalue) ] diff --git a/src/Soil-Core/SoilBTreeIterator.class.st b/src/Soil-Core/SoilBTreeIterator.class.st index f8a76a5c..077c6274 100644 --- a/src/Soil-Core/SoilBTreeIterator.class.st +++ b/src/Soil-Core/SoilBTreeIterator.class.st @@ -13,8 +13,8 @@ SoilBTreeIterator >> basicAt: key put: anObject [ ] { #category : #private } -SoilBTreeIterator >> findPageFor: key [ - ^currentPage := index rootPage find: key with: index +SoilBTreeIterator >> findPageFor: binKey [ + ^currentPage := index rootPage find: binKey with: index ] { #category : #accessing } diff --git a/src/Soil-Core/SoilBasicBTree.class.st b/src/Soil-Core/SoilBasicBTree.class.st index 75a6d009..f19d3ec0 100644 --- a/src/Soil-Core/SoilBasicBTree.class.st +++ b/src/Soil-Core/SoilBasicBTree.class.st @@ -100,18 +100,6 @@ SoilBasicBTree >> readPageFrom: aStream [ ^ SoilBTreePage readPageFrom: aStream keySize: self keySize valueSize: self valueSize ] -{ #category : #removing } -SoilBasicBTree >> removeKey: aString ifAbsent: aBlock [ - | page index key | - key := (aString asIndexKeyOfSize: self keySize) asInteger. - page := self newIterator - find: key; - currentPage. - ^ ((index := page indexOfKey: key) > 0) - ifTrue: [ (page itemRemoveIndex: index) value ] - ifFalse: [ aBlock value ] -] - { #category : #accessing } SoilBasicBTree >> rootPage [ ^ self store pageAt: 2 diff --git a/src/Soil-Core/SoilBasicSkipList.class.st b/src/Soil-Core/SoilBasicSkipList.class.st index 01c1cf19..4604339f 100644 --- a/src/Soil-Core/SoilBasicSkipList.class.st +++ b/src/Soil-Core/SoilBasicSkipList.class.st @@ -27,18 +27,6 @@ SoilBasicSkipList >> newIterator [ ^ SoilSkipListIterator on: self ] -{ #category : #removing } -SoilBasicSkipList >> removeKey: aString ifAbsent: aBlock [ - | page index key | - key := (aString asIndexKeyOfSize: self keySize) asInteger. - page := self newIterator - find: key; - currentPage. - ^ ((index := page indexOfKey: key) > 0) - ifTrue: [ (page itemRemoveIndex: index) value ] - ifFalse: [ aBlock value ] -] - { #category : #private } SoilBasicSkipList >> splitPage: aIterator forKey: aKey [ | newPage page | diff --git a/src/Soil-Core/SoilIndex.class.st b/src/Soil-Core/SoilIndex.class.st index 6589ea4f..cff79c0e 100644 --- a/src/Soil-Core/SoilIndex.class.st +++ b/src/Soil-Core/SoilIndex.class.st @@ -52,9 +52,9 @@ SoilIndex >> do: aBlock [ ] { #category : #private } -SoilIndex >> find: aString [ +SoilIndex >> find: key [ ^ self newIterator - find: (aString asIndexKeyOfSize: self keySize) asInteger + find: key ] { #category : #accessing } @@ -172,8 +172,14 @@ SoilIndex >> removeKey: key [ ] { #category : #'instance creation' } -SoilIndex >> removeKey: aString ifAbsent: aBlock [ - ^ self subclassResponsibility +SoilIndex >> removeKey: key ifAbsent: aBlock [ + | page index | + page := self newIterator + find: key; + currentPage. + ^ ((index := page indexOfKey: (key asIndexKeyOfSize: self keySize) asInteger) > 0) + ifTrue: [ (page itemRemoveIndex: index) value ] + ifFalse: [ aBlock value ] ] { #category : #accessing } diff --git a/src/Soil-Core/SoilIndexIterator.class.st b/src/Soil-Core/SoilIndexIterator.class.st index ba1a95d6..91097192 100644 --- a/src/Soil-Core/SoilIndexIterator.class.st +++ b/src/Soil-Core/SoilIndexIterator.class.st @@ -41,8 +41,7 @@ SoilIndexIterator >> at: aKeyObject [ { #category : #accessing } SoilIndexIterator >> at: aKeyObject ifAbsent: aBlock [ | foundValue | - currentKey := (aKeyObject asIndexKeyOfSize: index keySize) asInteger. - foundValue := self find: currentKey ifAbsent: aBlock. + foundValue := self find: aKeyObject ifAbsent: aBlock. ^(self restoreValue: foundValue forKey: currentKey) ifNil: [ aBlock value ] ] @@ -137,13 +136,13 @@ SoilIndexIterator >> find: key [ { #category : #private } SoilIndexIterator >> find: key ifAbsent: aBlock [ - currentKey := key. - self findPageFor: key. - ^ currentPage valueAt: key ifAbsent: aBlock + currentKey := (key asIndexKeyOfSize: index keySize) asInteger. + self findPageFor: currentKey. + ^ currentPage valueAt: currentKey ifAbsent: aBlock ] { #category : #private } -SoilIndexIterator >> findPageFor: key [ +SoilIndexIterator >> findPageFor: binKey [ ^ self subclassResponsibility ] @@ -279,7 +278,7 @@ SoilIndexIterator >> nextAssociation [ { #category : #accessing } SoilIndexIterator >> nextAssociationAfter: key [ - self find: (key asIndexKeyOfSize: index keySize) asInteger. + self find: key. ^ self nextAssociation ] diff --git a/src/Soil-Core/SoilSkipListIterator.class.st b/src/Soil-Core/SoilSkipListIterator.class.st index 3a851620..79ece122 100644 --- a/src/Soil-Core/SoilSkipListIterator.class.st +++ b/src/Soil-Core/SoilSkipListIterator.class.st @@ -33,7 +33,7 @@ SoilSkipListIterator >> basicAt: key put: anObject [ ] { #category : #private } -SoilSkipListIterator >> findPageFor: key [ +SoilSkipListIterator >> findPageFor: binKey [ | pageIndex candidatePage | currentPage := index headerPage. levels size to: 1 by: -1 do: [ :level | @@ -41,7 +41,7 @@ SoilSkipListIterator >> findPageFor: key [ pageIndex := currentPage rightAt: level. (pageIndex > 0) and: [ candidatePage := self pageAt: pageIndex. - candidatePage smallestKey <= key ] ] + candidatePage smallestKey <= binKey ] ] whileTrue: [ currentPage := candidatePage ]. self atLevel: level put: currentPage. ]. ^ currentPage