Various small improvements to recursive normalizers #2378
Conversation
…r's metastructure" This reverts commit 450170d948bf222f6fff53fef1290f102dafe5cf.
…not be supported as symbol names
|
On reflection I will add a change to 9ff3e94 to give people a way to disable the validation against |
|
Not ready for review - tests are broken |
|
|
||
| if depth > DEFAULT_RECURSE_LIMIT // 2: | ||
| raise DataTooNestedException(f"Symbol {original_symbol} cannot be recursively normalized as it contains more than " | ||
| f"{DEFAULT_RECURSE_LIMIT // 2} levels of nested dictionaries. This is a limitation of the msgpack serializer.") |
There was a problem hiding this comment.
I don't understand the factor of 2, so probably worth a comment
| @@ -19,20 +19,20 @@ namespace stream { | |||
| void register_stream_bindings(py::module &m); | |||
|
|
|||
| inline void register_bindings(py::module &m) { | |||
| auto arcticxx_types = m.def_submodule("types", R"pydoc( | |||
| auto arcticdb_ext_types = m.def_submodule("types", R"pydoc( | |||
There was a problem hiding this comment.
Will the bridge automagically handle these changes?
There was a problem hiding this comment.
This is just renaming the variable - so no impact on our APIs
| std::string msg_; | ||
| }; | ||
|
|
||
| using CheckOutcome = std::variant<Error, std::monostate>; |
There was a problem hiding this comment.
Not needed now, but this would be more generally useful if templated on the second variant type, so it could be used as the return type from non-void functions as well
The first commit 630de4 just shows how to fix an existing TODO in the code to make the Flattener non-recursive. It turns out that
msgpack.packbwhich we call straight afterwards is itself recursive as it serializes nested objects, so this change doesn't add any value and I just revert it in 2b8db. The git history is like this so that the reverted commit is still available in the history for people.b287372 introduces a clearer error message when we hit that msgpack recursion limit.
c872353 just fixes a comment that misunderstood what
pickle_on_failurecontrols.Recursive normalizers rely on serializing data using a separator of
__which meant that dictionaries with keys containing__did not roundtrip correctly (typically the__part of the keys would vanish upon read). 9ff3e94 validates against writing dictionaries with such a key. Alex and I discussed a lot about introducing a new serialization format that could handle these and decided against it - we can always do so in future.ea0b066 widens the set of dict keys that we can write. We were previously applying our symbol name validation to them, which is stronger than necessary.