feat(nano): support key mangling for nested container fields#1312
feat(nano): support key mangling for nested container fields#1312
Conversation
|
| Branch | feat/nested-container-fields |
| Testbed | ubuntu-22.04 |
🚨 1 Alert
| Benchmark | Measure Units | View | Benchmark Result (Result Δ%) | Upper Boundary (Limit %) |
|---|---|---|---|---|
| sync-v2 (up to 20000 blocks) | Latency minutes (m) | 📈 plot 🚷 threshold 🚨 alert (🔔) | 2.02 m(+20.46%)Baseline: 1.68 m | 2.01 m (100.38%) |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result minutes (m) (Result Δ%) | Lower Boundary minutes (m) (Limit %) | Upper Boundary minutes (m) (Limit %) |
|---|---|---|---|---|
| sync-v2 (up to 20000 blocks) | 📈 view plot 🚷 view threshold 🚨 view alert (🔔) | 2.02 m(+20.46%)Baseline: 1.68 m | 1.51 m (74.71%) | 2.01 m (100.38%) |
364375c to
8093e9a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1312 +/- ##
==========================================
- Coverage 86.03% 85.83% -0.21%
==========================================
Files 432 430 -2
Lines 32822 32973 +151
Branches 5114 5161 +47
==========================================
+ Hits 28239 28301 +62
- Misses 3575 3639 +64
- Partials 1008 1033 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
647c6e0 to
254b387
Compare
254b387 to
3904b1b
Compare
3904b1b to
a4ce768
Compare
There was a problem hiding this comment.
I couldn't find tests that confirm that final key used to store compound types in the RocksDB storage.
There was a problem hiding this comment.
I'll add some tests for this.
There was a problem hiding this comment.
Test will be added in the next PR.
a857f6f to
ae0fe2f
Compare
ae0fe2f to
2cbccd6
Compare
| try: | ||
| self.dc['foo'] | ||
| except KeyError as e: | ||
| assert e.args[0] == b'dc:\x03foo' |
There was a problem hiding this comment.
Shouldn't we assert pytest.raises(KeyError) for this case?
There was a problem hiding this comment.
This doesn't happen because of the implicit creation.
There was a problem hiding this comment.
We should remember to readd this when the implicit creation is removed, then
2cbccd6 to
462329a
Compare
462329a to
1c5fe8f
Compare
1c5fe8f to
a3ee9e2
Compare
a3ee9e2 to
c52cfd2
Compare
c52cfd2 to
eda7f6f
Compare
eda7f6f to
95b75f6
Compare
Motivation
Before this PR, compound containers fields will fully serialize the whole contents of contained fields:
Would mean each
list[bytes]is fully serialized, which hinders large lists.This PR makes it so the
list[bytes]above is serialized as arecords: list[bytes]field would be (using aDequeContainer), but using a dynamic prefix, instead of a prefix that only uses the field name, so(field_name, key)would become the prefix used byDequeContainer.Impact
Currently we support 3 types of containers (before this PR, they were only "attached" directly to fields), they are:
DequeContainer,DictContainer,SetContainer. Out of this 3, only aDequeContaineris iterable.By making inner containers also use our container implementations, some of them will not behave like the native Python type that it is replacing. For example:
In the blueprint above,
distances['foo']would return adict[str, int]which isIterable, however after this PR it will return something equivalent to aDictContainer, whic is notIterable. That is, previouslydistances['foo'].keys()would work, but after this PR it will no longer be the case.The same happens for a
set[T], but in this case the old behavior can be achieved usingfrozenset[T]instead. Adicthowever, doesn't really have a "frozen" equivalent, amappingproxycomes close, but we don't allow it to be imported, in theory we could support something likemappingproxy[K, V].Another impact is that
foo: tuple[dict[K, V]]will make it sofoo[0]still has the old behavior. But it's planned that aTupleContaineris introduced to makefoo[0]return aDictContainer.Acceptance Criteria
Fieldnot generic anymore, now it is the only concrete field type that carries aContainerNodeFactoryContainerNodeis an abstract type that can be either aContainerProxyor aContainerLeaf, aContainerNodeis never directly accessible in Blueprint codeContainerProxyimplements operations that compound the db-prefix until the operations reach aContainerLeafContainerLeafimplements operations on a specificNCTypethat actually stores data in the contract storageContainerabstract some common functionality that all containers have, whether a container uses aContainerNodedepends on the specific implementation, a concrete implementation of aContaineris what ends up being used in runtime when a blueprint doesself.foo[1]for example, so starting from aContainerit is important to not expose anything to blueprint code, this is achieved by only using double underscore properties (either__dunderfor private, implementation-specific properties; or__special__for properties that have to be accessible to some other class/module but not the blueprint)SetContainerusesContainerLeafdirectly, not aContainerNode, because it doesn't allow its members to mutateDequeContaineruses aContainerNodeto represent its members, it doesn't know directly whether a member is aContainerProxyor aContainerNode, but it operates on them through the abstract operations of aContainerNodeDictContaineruses a bareNCTypeto serialize keys (since they are only needed for generating the prefix for the values) and aContainerNodeto work with its values, similarly to how aDequeContainerdoes itChecklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged