feat: iterate over more than one prefix (#109) #127
Conversation
This file contains 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
prepare to create prefixed_items_basecoro using the same code
keep set for prefixed_items
Reviewer's Guide by SourceryThis PR adds support for iterating over multiple prefixes in ijson by introducing a new Class diagram for the new prefixed_items functionalityclassDiagram
class ItemsCommonBasecoro {
+builder_t builder
+int pending_builder_reset
+PyObject *target_send
+PyObject *prefix
+int object_depth
+yajl2_state *module_state
}
class PrefixedItemsBasecoro {
+PyObject *target_send
+PyObject *prefix
+int object_depth
+int pending_builder_reset
+builder_t builder
+yajl2_state *module_state
+PyObject* prefixed_items_basecoro_send_impl(PyObject *self, PyObject *path, PyObject *event, PyObject *value)
}
class ItemsBasecoro {
+PyObject *target_send
+PyObject *prefix
+int object_depth
+int pending_builder_reset
+builder_t builder
+yajl2_state *module_state
}
ItemsCommonBasecoro <|-- PrefixedItemsBasecoro
ItemsCommonBasecoro <|-- ItemsBasecoro
PrefixedItemsBasecoro <|-- PrefixedItemsGen
PrefixedItemsBasecoro <|-- PrefixedItemsAsync
class PrefixedItemsGen {
+reading_generator_t reading_gen
}
class PrefixedItemsAsync {
+async_reading_generator *reading_generator
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @wodny - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| N_N(retval = items_common_send_top(self, path, event, value)); | ||
| if (retval == Py_None) | ||
| Py_RETURN_NONE; | ||
| PyObject *tuple = PyTuple_Pack(2, path, retval); |
There was a problem hiding this comment.
suggestion (bug_risk): Add NULL check after PyTuple_Pack
PyTuple_Pack can return NULL on error. Should check the return value and handle the error case appropriately.
Suggested change
| PyObject *tuple = PyTuple_Pack(2, path, retval); | |
| PyObject *tuple = PyTuple_Pack(2, path, retval); | |
| if (tuple == NULL) { | |
| return NULL; | |
| } |
There was a problem hiding this comment.
This is correct, but it is not the only place with lacking checks (kvitems_basecoro.c, parse_basecoro.c). Probably a separate commit correcting all of them at once would be beneficial.
There was a problem hiding this comment.
I have created a GitHub issue for your comment: #128
| @@ -244,6 +252,14 @@ def _items_pipeline(backend, prefix, map_type, config): | |||
| ) | |||
|
|
|||
|
|
|||
| def _prefixed_items_pipeline(backend, prefix, map_type, config): | |||
There was a problem hiding this comment.
issue (complexity): Consider consolidating the parallel implementations into a single core prefixed pipeline with wrappers for non-prefixed functionality
The parallel implementations for prefixed and non-prefixed items create unnecessary complexity. We can simplify by making the prefixed version the core implementation and removing duplicate pipeline definitions. Here's how:
def _prefixed_items_pipeline(backend, prefix, map_type, config):
return (
(backend['prefixed_items_basecoro'], (prefix,), {'map_type': map_type}),
(backend['parse_basecoro'], [], {}),
(backend['basic_parse_basecoro'], [], config)
)
def _items_pipeline(backend, prefix, map_type, config):
# Reuse prefixed pipeline with wrapper to strip prefix
@utils.coroutine
def strip_prefix(target):
while True:
prefix, value = (yield)
target.send(value)
return _prefixed_items_pipeline(backend, prefix, map_type, config)[:-1] + \
((strip_prefix, [], {}),)This approach:
- Makes prefixed_items_pipeline the core implementation
- Removes duplicate pipeline logic
- Maintains all functionality while reducing code complexity
- Keeps the separation between prefixed and non-prefixed interfaces
The same pattern can be applied to the generator and async implementations.
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.
I prepared some proposals to check if the way I tackle the problem from #109 is acceptable.
I implemented the changes in Python and
yajl2_cbackends.The commit stack contains two variants:
items(),items()and the newprefixed_items(), but has a drawback of performing 10% worse than the original.I benchmarked the code both using defaults and some input I parse in production which is a map of very long lists of maps for which using
items()is essential.Both versions use
items_common.hwhich containsstatic inlinefunctions used byitems()and the newprefixed_items(). An analogy to thebuilder.hfile. The code is all that I was able to move fromitems_basecoro.c. Because the code is shared, in the non-set-items()variant a macro is used to define the comparison method (PyObject_RichCompareBoolvsPySet_Contains). The shared function is divided into top and bottom halves so the calling side can do a specializedCORO_SENDin between and avoid mangling a generic tuple. Division into halves required one additional state information -pending_builder_reset.Before I did the
items_common.hI tried extending pipelines but the built-in tests changed my mind. All elements of the pipelines have input and output formats already defined and eg. the items basecoro is tested directly with events as inputs.I think I managed to avoid additional tuple creation/destruction with the exception of the pure Python code where I assumed it won't matter. I can always write two specialized functions.
I extended the list of
ifs inparse_basecoro.cto detect the newPrefixedItemsBasecoro:Everything else is just copying
items*.{c,h}as a template and adding theprefixedprefix ;) I made it in a separate commit ("add prefixed_items") because it's the biggest one and I didn't want to mix it with the actual changes.All tests (including memleaks) pass without modification.
I did not touch the
kvitems()part yet nor did I write any tests forprefixed_items()in case you decide the changes should be done in a completely different manner.Summary by Sourcery
Add support for iterating over multiple prefixes in the ijson library, enhancing data parsing capabilities. This includes new coroutine and generator implementations for prefixed_items, with shared code to maintain efficiency.
New Features:
Enhancements: