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

types: add dict views #2358

Merged
merged 1 commit into from
May 11, 2022
Merged

types: add dict views #2358

merged 1 commit into from
May 11, 2022

Conversation

davidhewitt
Copy link
Member

Inspired by #2347, having a play with adding the dictionary view types.

TODO

  • What should their full API be? E.g. impl IntoIterator?
  • Do we want to ask upstream CPython for a C-API rather than having to call by (interned) string method name?
  • CHANGELOG entry

@samuelcolvin
Copy link
Contributor

Thank you so much.

Just a thought - not particularly necessary for me, would it be possible to add methods to PyDict to return keys as a PyDictKeys etc?

E.g. .dict_keys()?

pub fn keys_view(&self) -> &PyDictKeys {
unsafe {
PyDictKeys::try_from_unchecked(
self.call_method0(intern!(self.py(), "keys")).unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can self be a dict subclass -- which can therefore return a different type?

Copy link
Member Author

@davidhewitt davidhewitt May 5, 2022

Choose a reason for hiding this comment

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

Yes, good point, this unchecked is not sound. I guess that would be the advantage of having a C API - then we could always hit the guaranteed pathway rather than lookup and allow subclasses to override things.

@davidhewitt
Copy link
Member Author

Just a thought - not particularly necessary for me, would it be possible to add methods to PyDict to return keys as a PyDictKeys etc?

E.g. .dict_keys()?

I think this is what I've called PyDict::keys_view etc. in this PR? Though I might punt on the methods for now and just add the type definitions as a first step.

@mejrs
Copy link
Member

mejrs commented May 5, 2022

Is it safe to have a dictview while mutating the dict itself?

@davidhewitt
Copy link
Member Author

Is it safe to have a dictview while mutating the dict itself?

Yes, looks like it:

>>> d = {}
>>> k = d.keys()
>>> k
dict_keys([])
>>> d['foo'] = 1
>>> k
dict_keys(['foo'])

@davidhewitt davidhewitt force-pushed the dict-views branch 2 times, most recently from 9b25284 to 4ad66b5 Compare May 11, 2022 03:08
@davidhewitt
Copy link
Member Author

davidhewitt commented May 11, 2022

I've pared this back to just be the struct definitions for now; the decision on what to do about methods might depend on what upstream API CPython will accept. They probably won't be supported until Python 3.12 though, I would now assume.

@davidhewitt davidhewitt marked this pull request as ready for review May 11, 2022 03:09
@davidhewitt davidhewitt force-pushed the dict-views branch 2 times, most recently from ee5a692 to c091318 Compare May 11, 2022 17:43
@samuelcolvin
Copy link
Contributor

For my use case, this looks great. Thanks so much.

@davidhewitt
Copy link
Member Author

For my use case, this looks great. Thanks so much.

👍. Worth noting that it appears these types appear not to be supported on pypy, although they would probably be willing to add if requested on their issue tracker.

@davidhewitt davidhewitt merged commit 1482b52 into PyO3:main May 11, 2022
@davidhewitt davidhewitt deleted the dict-views branch May 11, 2022 19:34
@samuelcolvin
Copy link
Contributor

Is there any chance that iterating over dict items will be faster than the list I get from .items()?

@davidhewitt
Copy link
Member Author

Off the top of my head I'm not sure I can call it either way. Iterating lists is probably faster than iterating the keys collection, but you've got to pay the upfront cost of allocating and filling the whole list. It probably depends on dictionary size - as always, best to benchmark for your use case 😊

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.

4 participants