Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions python/arcticdb/flattener.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@
from arcticdb.exceptions import DataTooNestedException, UnsupportedKeyInDictionary

try:
from msgpack.fallback import DEFAULT_RECURSE_LIMIT
from msgpack.fallback import DEFAULT_RECURSE_LIMIT as _DEFAULT_RECURSE_LIMIT
except ImportError:
# The default as of msgpack 1.1.0 - handle the import error in case the msgpack wheel stops exporting this constant.
# Want to keep compatibility with a wide range of msgpack versions.
DEFAULT_RECURSE_LIMIT = 511
_DEFAULT_RECURSE_LIMIT = 511

from arcticdb import _msgpack_compat
from arcticdb.log import version as log
Expand All @@ -27,6 +27,16 @@
from arcticdb.preconditions import check
from arcticdb_ext import get_config_int

# The binding constraint is the read path: reconstructing the data recurses ~3 Python stack frames
# per nesting level (see create_original_obj_from_metastruct), so with the default recursion limit of
# 1000 reads fail above ~330 levels. We pin the cap at 255 - the value it was effectively fixed at for
# years, when msgpack's DEFAULT_RECURSE_LIMIT was 511 (511 // 2 == 255) - which both packs on write and
# round-trips on read. The min() keeps msgpack's packer as a ceiling too. Tying the cap directly to
# DEFAULT_RECURSE_LIMIT broke when msgpack 1.2.0 raised it to 1024, letting writes through at depths the
# reader could not reconstruct.
# https://man312219.monday.com/boards/7852509418/pulses/12254825163
DEFAULT_RECURSE_LIMIT = min(511, _DEFAULT_RECURSE_LIMIT)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I find this explanation confusing. The recursion limit has always been 1000

As far as I understand the problem is that our metastruct reader uses 3 stackframe recursion so we reach the limit at around 330.

Should we just set the threshold for nesting to something like (DEFAULT_RECURSION_LIMIT - 10) // 3? The 10 is for the stack frames which call the decoding code.

After we fix our metastruct reading to not be recursive we will be able to return it to the (DEFAULT_RECURSION_LIMIT - 10) // 2

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Anyway I'm fine with the current fix to unblock the CI and it essentially preserves the old limit.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I have updated the comment to better explain the write and read limits



class Flattener:
# Probably a bad idea given the dict key could have this, fine for now as write does not allow this symbol anyways.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import numpy as np
import arcticdb
from arcticdb import QueryBuilder, LibraryOptions
from arcticdb.flattener import Flattener
from arcticdb.flattener import Flattener, DEFAULT_RECURSE_LIMIT
from arcticdb.version_store._custom_normalizers import (
CustomNormalizer,
register_normalizer,
Expand Down Expand Up @@ -392,12 +392,18 @@ def test_deep_nesting_metastruct_size_over_limit(lmdb_version_store_v1, all_recu
key = "reasonable_length_key"
data = {key: pd.DataFrame({"col": [0]})}

nesting_levels = 256
# Mirror the guard in flattener._create_meta_structure (raises above DEFAULT_RECURSE_LIMIT // 2).
# flattener clamps DEFAULT_RECURSE_LIMIT to 511, so this is 255 - see the note there for why.
nesting_limit = DEFAULT_RECURSE_LIMIT // 2
nesting_levels = nesting_limit + 1
for i in range(nesting_levels - 1):
data[key] = {key: data[key]}

# When & Then
with pytest.raises(DataTooNestedException, match=r"^Symbol sym cannot be recursively normalized.*255 levels.*"):
with pytest.raises(
DataTooNestedException,
match=rf"^Symbol sym cannot be recursively normalized.*{nesting_limit} levels.*",
):
lib.write(sym, data, recursive_normalizers=True)


Expand All @@ -408,7 +414,7 @@ def test_deep_nesting_metastruct_size_under_limit(lmdb_version_store_v1, all_rec
key = "reasonable_length_key"
data = {key: pd.DataFrame({"col": [0]})}

nesting_levels = 255
nesting_levels = (DEFAULT_RECURSE_LIMIT - 1) // 2
for i in range(nesting_levels - 1):
data[key] = {key: data[key]}

Expand Down
Loading