Support negative indices in serializer include/exclude #627
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #627 +/- ##
==========================================
+ Coverage 94.02% 94.04% +0.01%
==========================================
Files 98 98
Lines 13283 13324 +41
Branches 25 25
==========================================
+ Hits 12490 12531 +41
Misses 787 787
Partials 6 6
Continue to review full report in Codecov by Sentry.
|
CodSpeed Performance ReportMerging #627 Summary
Benchmarks breakdown
|
src/serializers/filter.rs
Outdated
| fn map_negative_indices<'a, 'py: 'a>( | ||
| py: Python<'py>, | ||
| include_or_exclude: &'a PyAny, | ||
| len: Option<usize>, | ||
| ) -> PyResult<&'a PyAny> { | ||
| if let Ok(exclude_dict) = include_or_exclude.downcast::<PyDict>() { | ||
| let out = PyDict::new(py); | ||
| for (k, v) in exclude_dict.iter() { | ||
| out.set_item(map_negative_index(py, k, len)?, v)?; | ||
| } | ||
| Ok(out) | ||
| } else if let Ok(exclude_set) = include_or_exclude.downcast::<PySet>() { | ||
| let mut values = Vec::with_capacity(exclude_set.len()); | ||
| for v in exclude_set { | ||
| values.push(map_negative_index(py, v, len)?) | ||
| } | ||
| Ok(PySet::new(py, values)?) | ||
| } else { | ||
| // return as is and deal with the error later | ||
| Ok(include_or_exclude) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is not particularly efficient. We're downcasting twice (once here, again later) and re-creating the entire iterable (when really we could do it on the fly). One option to change this would be to duplicate the "hellish logic" below (which I wanted to avoid) to handle indexes and keys/fields separately. It might also be good to extract the entirety of include and exclude into rust types upfront so that it's not only cheaper to do this post-processing but also maybe some of the "hellish logic" becomes simpler or at least more understandable via type hints.
A quick win might be to check if there are negative indexes in a first pass and only map them / copy the input if there are. I'm skipping that for now since it requires benchmarks to prove it's even better and is a lot more code/boilerplate.
I think this is good enough (and fast enough) for now, we can come back to those more advanced options later.
|
please review cc @dmontagu |
|
Seems like this is currently fixing https://github.com/pydantic/pydantic/blob/3ffb0c465a2d30accacbde1509ced0a74ba391e5/tests/test_edge_cases.py#L603 but not the other tests. Need to investigate a bit further (and maybe port them over here) |
|
@adriangb I think I've the extra logic should fix more cases. Could you check? Feel free to merge if you're happy. |
|
Yes will look later tonight / tomorrow morning |
There was a problem hiding this comment.
I would prefer to remove map_negative_indices, but feel free to merge if it's working everywhere.
Instead of calling __mod__ on each element the include/exclude set/dict, could we instead do the equivilant of
if thing := filter.get(index):
return thing
elif thing := filter.get(index - len):
return thingE.g. move the conversion logic from the include/exclude argument to the index value.
And thereby avoid re-creating the set and dict? Just an idea for the future.
|
I tested in Pydantic and this is now resolving all of the tests there so I'm going to go ahead and merge this. Agreed that there may be better ways but none of them are obviously faster or are much more complex, this seems good enough for now. |
Closes pydantic/pydantic#5838
Selected Reviewer: @dmontagu