tidy up tagged_union_schema
#1333
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CodSpeed Performance ReportMerging #1333 will not alter performanceComparing Summary
|
| @@ -71,7 +70,7 @@ impl<T: Debug> LiteralLookup<T> { | |||
| .map_err(|_| py_schema_error_type!("error extracting str {:?}", k))?; | |||
| expected_str.insert(str.to_string(), id); | |||
| } else if expected_py_dict.set_item(&k, id).is_err() { | |||
| expected_py_list.append((&k, id))?; | |||
| expected_py_values.push((k.as_unbound().clone_ref(py), id)); | |||
There was a problem hiding this comment.
Just trying to learn more about rust syntax here - could you explain this as_unbound.clone_ref(py)? Thanks!
There was a problem hiding this comment.
As discussed offline - is PyO3 code to change from Bound<'py, PyAny> to Py<PyAny>.
| @@ -35,7 +34,7 @@ pub struct LiteralLookup<T: Debug> { | |||
| // Catch all for hashable types like Enum and bytes (the latter only because it is seldom used) | |||
| expected_py_dict: Option<Py<PyDict>>, | |||
| // Catch all for unhashable types like list | |||
| expected_py_list: Option<Py<PyList>>, | |||
| expected_py_values: Option<Vec<(Py<PyAny>, usize)>>, | |||
There was a problem hiding this comment.
I'm assuming this might be more performant - are there any benchmarks that reflect that?
There was a problem hiding this comment.
No benchmarks, but otherwise yes, we don't need to make Python FFI calls here to maintain this internal state :)
sydney-runkle
approved these changes
Jun 17, 2024
This was referenced Jul 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #925
While checking out the suggested type hints there and comparing to the implementation, I saw that we stick all unhashable discriminants in a list and to equality comparison on them. So I decided to keep things simple and relax the
Hashablebound toAny.At the same time, I saw that the list was not necessary and we could use a Rust
Vec. So I simplified the implementation a little.