Skip to content

Weird level-deletion in intersectionArrayBy #420

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

Closed
sjakobi opened this issue Apr 17, 2022 · 6 comments · Fixed by #427
Closed

Weird level-deletion in intersectionArrayBy #420

sjakobi opened this issue Apr 17, 2022 · 6 comments · Fixed by #427
Labels

Comments

@sjakobi
Copy link
Member

sjakobi commented Apr 17, 2022

case len of
0 -> pure Empty
1 -> A.read mary 0
_ -> bitmapIndexedOrFull bFinal <$> (A.unsafeFreeze =<< A.shrink mary len)

I'm referring to line 1864 where, instead of returning a BitmapIndexed node, its only element is returned.

@oberblastmeister Could you clarify the reasoning behind this code?

I'm wondering whether this might mess with the shift/subkey-logic, effectively misplacing some elements, so they can't be found by lookups, deletions etc.

@sjakobi
Copy link
Member Author

sjakobi commented Apr 17, 2022

As a precaution, I've deprecated v0.2.19.0 on Hackage. I can un-deprecate it if it turns that this code is correct.

@sjakobi sjakobi added the bug label Apr 17, 2022
@oberblastmeister
Copy link
Contributor

oberblastmeister commented Apr 17, 2022

Why would this mess with shift/subkey-logic? This is done in other places of the code already, such as delete. Moreover, it has to be done whenever items could be removed, in order to satisfy the invariants in #366.

@oberblastmeister
Copy link
Contributor

I think this may be wrong, because we need to check if it is a Leaf or Collision, like in delete.

@sjakobi
Copy link
Member Author

sjakobi commented Apr 20, 2022

I think this may be wrong, because we need to check if it is a Leaf or Collision, like in delete.

Exactly!

sjakobi added a commit that referenced this issue Apr 20, 2022
sjakobi added a commit that referenced this issue Apr 20, 2022
sjakobi added a commit that referenced this issue Apr 20, 2022
@treeowl
Copy link
Collaborator

treeowl commented Apr 20, 2022

It is a bit curious that we don't have general path compression. I suppose it's not supposed to be too important if the hashing is good? Of course, instances like Hashable Int are very not good, so we might want to experiment. OTOH, it might be better to just write a different data structure for Int. @wrengr was talking about non-hashed array-mapped tries at one point; I don't know how far she got exploring that.

@sjakobi
Copy link
Member Author

sjakobi commented Apr 20, 2022

@treeowl Let's not discuss design issues on a minor bug ticket. Feel free to investigate #391 if you like that idea.

sjakobi added a commit that referenced this issue Apr 24, 2022
sjakobi added a commit that referenced this issue Apr 24, 2022
Fixes #420.

Co-authored-by: Brian Shu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants