Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added iterator remove, re-arranged some functions in InternaOakHash/Map/Basic #191

Merged
merged 32 commits into from
Jan 16, 2022

Conversation

boris-kap
Copy link
Contributor

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

Copy link
Contributor

@liran-funaro liran-funaro left a comment

Choose a reason for hiding this comment

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

Please address these comments.

core/src/main/java/com/yahoo/oak/InternalOakBasics.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/InternalOakMap.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/InternalOakMap.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/InternalOakMap.java Outdated Show resolved Hide resolved
Copy link
Contributor

@sanastas sanastas left a comment

Choose a reason for hiding this comment

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

The fix looks good to me. Multiple comments below, however. Also I think it needs to be added after PR#196.

core/src/main/java/com/yahoo/oak/InternalOakBasics.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/InternalOakBasics.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/InternalOakHash.java Outdated Show resolved Hide resolved
Comment on lines 528 to 544
@Override
public void remove() {
if (!isPrevIterStateValid()) {
throw new IllegalStateException("next() was not called in due order");
}
BasicChunk prevChunk = getPrevIterState().getChunk();
int preIdx = getPrevIterState().getIndex();
boolean validState = prevChunk.readKeyFromEntryIndex(ctx.key, preIdx);
if (validState) {
K prevKey = getKeySerializer().deserialize(ctx.key);
InternalOakHash.this.remove(prevKey, null, null);


}

invalidatePrevState();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I also do not see the reason for overriding

Copy link
Contributor

@sanastas sanastas left a comment

Choose a reason for hiding this comment

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

Looks good, but I still some comments. Please address. Thanks!

* advance state to the new position
* @return if new position found, return true, else, set State to null and return false
*/
protected boolean advanceState() {
Copy link
Contributor

Choose a reason for hiding this comment

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

In case something is changed in advance() it might need to affect advanceStream() as well

core/src/main/java/com/yahoo/oak/InternalOakHash.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/InternalOakMap.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/OakMap.java Outdated Show resolved Hide resolved
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