Fix implicitly bound resources to be able to reference MultiVector #18
Conversation
|
|
|
Looks good. I still would like @imagovrn to take a look. |
There was a problem hiding this comment.
Implicitly binding with Multivector was never intended. Multivector is an associative data structure, not a sequence. Implicitly binding two sequences is well defined, whereas interpreting multivectors' index can be done in many ways (it could, for instance, store all the items to support constant time lookup or just some of them - for binary search). So I'd say we shouldn't do it without thinking it over again.
Makes sense, after talking about it.
Please have a look at two comments in the code.
| @@ -15,9 +15,9 @@ def create(properties, own_schema): | |||
| own_schema=own_schema) | |||
|
|
|||
| def _create_references(self): | |||
| return [VectorReference(name=r) for r in self._resources] | |||
| return [ResourceReference(name=r) for r in self._resources] | |||
There was a problem hiding this comment.
I would not add ResourceReference here as it incldues raw_data and we surely can't implicitly bind to raw_data. How about adding SequentialResourceReference or something like that?
There was a problem hiding this comment.
I was thinking about that, and my reasoning was like this:
- RAW means the user manages the data
- The user might store data in RAW that use indexed
- The user might want to indicate in the schema that their RAW data is implicitly bound (however they manage their RAW data)
No strong opinions in that direction, but that seemed like a reasonable thing to allow (we do not benefit from being restrictive, do we?)
There was a problem hiding this comment.
Just a small note: Right now, we already allow to create explicit references from vectors into raw data.
| @@ -136,7 +136,7 @@ def test_duplicate_multivector_builtin_types_are_not_produced(): | |||
| @bound_implicitly( b: A0.v0, A0.v1 ) | |||
| archive A0 { | |||
| v0 : vector< S1 >; | |||
| v1 : vector< S1 >; | |||
| v1 : multivector< 14, S1 >; | |||
There was a problem hiding this comment.
Please add, not replace. Better - add another test. This one will check for vectors. Another - for multivectors.
There was a problem hiding this comment.
That is not a test for vector. This tree is named TREE_WITH_ALL_FEATURES
Fixes #17