Skip to content

Retain order of keys in dict when deleting a key #2550

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.

Sign up for GitHub

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
fermian wants to merge 4 commits into from
Closed

Retain order of keys in dict when deleting a key #2550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0344aff
Retain order of keys in dict when deleting one
fermian Apr 12, 2021
a5294fd
Update test_dict.py
fermian Apr 12, 2021
d4d41bb
Update items in indices vector when removing key
fermian Apr 12, 2021
f19ed42
Update test_collections.py
fermian Apr 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions Lib/test/test_collections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1974,8 +1974,6 @@ def test_init(self):
self.assertRaises(TypeError, Counter, (), ())
self.assertRaises(TypeError, Counter.__init__)

# TODO: RUSTPYTHON
@unittest.expectedFailure
def test_order_preservation(self):
# Input order dictates items() order
self.assertEqual(list(Counter('abracadabra').items()),
Expand Down
2 changes: 0 additions & 2 deletions Lib/test/test_dict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1346,8 +1346,6 @@ def iter_and_mutate():

self.assertRaises(RuntimeError, iter_and_mutate)

# TODO: RUSTPYTHON
@unittest.expectedFailure
def test_reversed(self):
d = {"a": 1, "b": 2, "foo": 0, "c": 3, "d": 4}
del d["foo"]
Expand Down
11 changes: 7 additions & 4 deletions vm/src/dictdatatype.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,10 +557,13 @@ impl<T: Clone> Dict<T> {
let removed = if entry_index == inner.used {
inner.entries.pop().unwrap()
} else {
let last_index = inner.entries.last().unwrap().index;
let removed = inner.entries.swap_remove(entry_index);

@youknowone youknowone Apr 14, 2021

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.

oh... I believed it is not swap_remove and we were doing it as how CPython does.

inner.indices[last_index] = entry_index as i64;
removed
let entries_rem = entry_index + 1;
let entries_len = inner.entries.len();
for move_entry_idx in entries_rem..entries_len {
let index_index = inner.entries[move_entry_idx].index;
inner.indices[index_index] = move_entry_idx as i64 - 1;
}
Comment on lines +562 to +565

@youknowone youknowone Apr 14, 2021

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.

I don't think CPython uses loop for this. What happens if the dict has very many items? Would you check _PyDict_Pop_KnownHash in dictobject.c? I think this is the corresponding funciton in CPython.

@DimitrisJim DimitrisJim Aug 14, 2021

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.

Yeah, this definitely won't be a loop, pretty sure that leads to O(N) deletes.

@youknowone youknowone Aug 14, 2021

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.

@eldpswp99 is working on this again

inner.entries.remove(entry_index)
};
Ok(Some(removed))
}
Expand Down