Add logical types. Minor improvements. #34

siilike · 2019-06-24T20:06:19Z

What's new:

logical types (see below);
immutable Buffers from pointers;
minor improvements to the Collections class.

From the documentation:

Types 0xee-0xef are used for tagging of values to implement logical
types.

For example, if type 0x1c did not exist, the database driver could
serialize a timestamp object (Date in JavaScript, Instant in Java, etc)
into a Unix timestamp, a 64-bit integer. Assuming the lack of schema,
upon deserialization it would not be possible to tell an integer from
a timestamp and deserialize the value accordingly.

Type tagging resolves this by attaching an integer tag to values that
can then be read when deserializing the value, e.g. that tag=1 is a
timestamp and the relevant timestamp class should be used.

The tag values are specified separately and applications can also
specify their own to have the database driver deserialize their specific
data types into the appropriate classes (including models).

Essentially this is object-relational mapping for parts of documents.

The format of the type is:
0xee TAG in 1 byte sub VPack value 
or
0xef TAG in 8 bytes sub VPack value 

Tagging is implemented transparently -- operations on a Slice are unaffected and performed on the value of the tagged type, not on the type itself.

Looking forward to having this merged quickly.

graetzer · 2019-06-27T15:27:29Z

can you give us a rational for implementing this into velocypack ? I feel this is a sbustantial change and we need to understand this better

graetzer · 2019-06-27T15:33:24Z

seems to me like the application should know the schema ahead of time, no ?

siilike · 2019-06-27T16:48:41Z

The use case for this is applications where schema is not explicitly defined or is defined only partially, so the database driver will need additional information to deserialize the values properly.

The timestamp issue given above is a classical example. It can be solved either by having a dedicated type (like VelocyPack has) or by having a tag that signals that the integer that has been stored is a timestamp. The latter way also allows defining application-specific types and types that would not be very useful at a format level (like an age or a height type).

This is highly useful in simpler applications where one can get away with not explicitly specifying the schema and different abstractions, basically the MongoDB use case. But it assumes that what goes into the database comes out of there with the exact same classes, otherwise it will get very clumsy.

Regarding immutable buffers, I checked and turns out I don't have a use case for them any more, not sure what exactly they were for. I will update the pull request accordingly.

siilike · 2019-07-31T20:15:39Z

Any updates on this?

jsteemann · 2019-09-03T10:29:42Z

@siilike : sorry, all blame is on me, because I forgot to have a look at this PR.
I will get back within the next 24 hours.

siilike · 2019-09-03T10:41:42Z

Great, there are some changes still missing to make it work with the latest changes in master.

siilike · 2019-09-03T12:07:24Z

Now it is all updated. The only downside of the current implementation is that Slice supports transparency for only one level of tags, otherwise every access to _start would require iteration.

VelocyPack.md

include/velocypack/Builder.h

include/velocypack/Slice.h

include/velocypack/velocypack-common.h

src/Collection.cpp

jsteemann · 2019-09-04T07:51:42Z

src/Collection.cpp

 return false;
 }

+bool Collection::containsObject(Slice const& slice, Slice const& other) {


should this be renamed to containsSubobject?

jsteemann · 2019-09-04T07:52:09Z

include/velocypack/Collection.h


 static bool contains(Slice const& slice, Slice const& other);

+ static bool containsObject(Slice const& slice, Slice const& other);


should this be renamed to containsSubobject?

I checked and I actually don't have any use for this method, so I removed it for now.

I was also thinking that it is rather confusing to have array and object methods in the same class as they are not compatible, e.g. it is not possible to filter() or concat() an object, so it would make sense to either add support for objects to all methods or create an entire separate class for handling the same operations for objects.

jsteemann · 2019-09-04T07:57:03Z

In general, I think the idea of tags makes sense for some applications.
However, this PR changes the behavior of several public methods in velocypack::Slice, so when existing applications start using tags, all existing calls in these applications to Slice::start(), Slice::byteSize() etc. will need to be changed.
Instead, the public methods in Slice should remain downwards-compatible. Otherwise, only the VPack data without tags will make it into client applications, and client applications have no chance of using the tags for anything.
Additionally, I now a few embedders that use the combination of start() and byteSize() and feed that into memcpy. If the behavior of start() and byteSize() however changes, then all tags will instantly be lost unless all embedder code is changed.

jsteemann · 2019-09-04T07:58:28Z

And finally, we must add tests for tagged values and for the new method in Collections before we can this PR.

jsteemann · 2019-09-04T08:05:23Z

if it helps, I am also happy to split up this multi-purpose PR into multiple small single-purpose PRs. That way we can already merge the following bits quickly, without having to wait for the bigger tags change:

the Collection-related additions (plus tests)
the 32bit hash function (plus tests)
CMakeLists.txt changes
WDYT?

siilike · 2019-09-04T09:29:07Z

Thank you for reviewing.

The idea of making the tags transparent by default was that it makes working with the values a lot simpler. Otherwise there would need to be a method that returns the value if tagged and otherwise the object itself which would clutter the code and be an easy source of errors if the value initially isn't tagged, but becomes at some point.

But the important part is that is* and get* methods apply to the value of the tagged value, not the tagged value itself, so start(), end() and others can be reverted to apply to the whole value. This way we'll have enough compatibility with existing solutions and convenient access to values.

Fix first issues Co-Authored-By: Jan <jsteemann@users.noreply.github.com>

siilike · 2019-09-04T12:29:03Z

I removed the exception from Slice.tagOffset, because the only time when there can be an error is when a new tagged type is added to SliceStaticData.TypeMap and that method is not updated. This removes all the headache of noexcept.

I now updated the transparency logic as follows:

methods that deal with the pointer and length keep the former behaviour and have a relevant value* method: start, startAs, head, begin, end, byteSize;
in methods that deal with the value tags are transparent and some have a relevant raw* method: type, typeName, is*, get*, copy*, has*, etc.

If that is fine I'll also update Slice.cpp and any references elsewhere.

siilike · 2019-09-10T12:04:25Z

ping

jsteemann · 2019-09-10T16:34:08Z

include/velocypack/Slice.h

- constexpr inline bool isType(ValueType t) const noexcept {
- return SliceStaticData::TypeMap[*start()] == t;
+ constexpr inline bool isType(ValueType t) const {
+ return SliceStaticData::TypeMap[*valueStart()] == t;


If this should include tags, then I think it should be

return SliceStaticData::TypeMap[*start()] == t;

include/velocypack/Slice.h

jsteemann · 2019-09-10T16:39:55Z

include/velocypack/Slice.h

+ // get the type for the slice (excluding tags)
 constexpr inline ValueType type() const noexcept {
- return SliceStaticData::TypeMap[head()];
+ return type(valueHead());


The changed implementation in the PR changes the behavior of this method to return the type after jumping over the tag. This will make it non-downwards compatible. I think it should be reverted to:

Suggested change

return type(valueHead());

return SliceStaticData::TypeMap[head()];

jsteemann · 2019-09-10T16:41:31Z

include/velocypack/Slice.h

- return SliceStaticData::TypeMap[h];
+
+ // get the type for the slice (including tags)
+ constexpr inline ValueType rawType() const noexcept {


This should probably be renamed to valueType and return the type of the tagged value.

jsteemann · 2019-09-10T19:54:56Z

include/velocypack/Slice.h


 // check if slice is the Boolean value true
- constexpr bool isTrue() const noexcept { return head() == 0x1a; }
+ constexpr bool isTrue() const noexcept { return valueHead() == 0x1a; }


I think this should still operate on head() and not on valueHead().
If a client application needs to peek into the value behind the tag, I think it should explicitly resolve the tag.

jsteemann · 2019-09-10T20:00:38Z

include/velocypack/Slice.h

 // return the value for a String object
 char const* getString(ValueLength& length) const {
- uint8_t const h = head();
+ uint8_t const h = valueHead();


I think this should still operate on head() and not on valueHead().
If a client application needs to peek into the value behind the tag, I think it should explicitly resolve the tag.

jsteemann · 2019-09-10T20:00:45Z

include/velocypack/Slice.h

 if (h == 0xbf) {
 // long UTF-8 String
- length = readIntegerFixed<ValueLength, 8>(_start + 1);
+ length = readIntegerFixed<ValueLength, 8>(valueStart() + 1);


I think this should still operate on start() and not on valueStart().
If a client application needs to peek into the value behind the tag, I think it should explicitly resolve the tag.

jsteemann · 2019-09-10T20:00:50Z

include/velocypack/Slice.h

+ length = readIntegerFixed<ValueLength, 8>(valueStart() + 1);
 checkOverflow(length);
- return reinterpret_cast<char const*>(_start + 1 + 8);
+ return reinterpret_cast<char const*>(valueStart() + 1 + 8);


I think this should still operate on start() and not on valueStart().
If a client application needs to peek into the value behind the tag, I think it should explicitly resolve the tag.

jsteemann · 2019-09-10T20:01:49Z

include/velocypack/Slice.h


 char const* getStringUnchecked(ValueLength& length) const noexcept {
- uint8_t const h = head();
+ uint8_t const h = valueHead();


I think this should still operate on head() and not on valueHead().
If a client application needs to peek into the value behind the tag, I think it should explicitly resolve the tag.

jsteemann · 2019-09-10T20:03:12Z

include/velocypack/Slice.h

 // short UTF-8 String
 length = h - 0x40;
- return reinterpret_cast<char const*>(_start + 1);
+ return reinterpret_cast<char const*>(valueStart() + 1);


I feel I am repeating the same sentence here over and over 😄
The same argument applies for most of the following functions.

siilike · 2019-09-15T08:52:12Z

Pushed the third take on this:

Slice has no behavioural changes, only tags support added;
created new ValueSlice class where tags are transparent;
added support for multiple tags -- initially I skipped this as I did not want to introduce iteration everywhere, but it seems lousy not to implement it when technically it is possible to have multiple tags.

If this is fine I'll also update Builder.

siilike · 2019-10-11T20:54:03Z

Turns out this overriding idea does not work. The start() method would have to be virtual which would mean the removal of constexpr and sizeof(arangodb::velocypack::Slice) != sizeof(void*).

coveralls · 2019-11-10T15:40:16Z

Coverage decreased (-1.2%) to 94.984% when pulling ac70e54 on siilike:siilike into 4ebe24e on arangodb:master.

siilike · 2019-11-10T15:47:06Z

OK, I got rid of ValueSlice for now as I can temporarily give up the convenience until I find the time to resolve this. So, please review the tests and merge if everything is fine.

OmarAyo · 2019-11-11T11:47:27Z

Hi @siilike

Many thanks for contributing. We are obliged by law to have a signed scanned copy of a Contributor License Agreement (CLA) to be able to accept pull requests to the official ArangoDB repository -

We use an Apache 2 CLA for ArangoDB and its companion projects, which can be found here: https://www.arangodb.com/documents/cla.pdf
Just fill in the form, sign and send a scanned copy over to cla@arangodb.com

Thank you

OmarAyo · 2019-11-15T11:06:55Z

CLA available

siilike · 2019-11-21T07:56:03Z

Ping

siilike · 2019-11-28T09:47:34Z

Still waiting.

siilike · 2019-12-09T09:34:11Z

Yet another week has gone by.

jsteemann · 2019-12-09T09:36:28Z

@siilike : Hi, sorry for the delay. I am currently busy with lots of other things as we are preparing a release. I will make time to look into this on either Wed or Thu this week. Sorry again!

siilike · 2019-12-09T09:57:43Z

Great, thanks.

I made a separate pull request for the specification change, so we can continue with arangodb/java-velocypack#15 in the meantime.

src/Dumper.cpp

include/velocypack/velocypack-memory.h

src/Builder.cpp

jsteemann · 2019-12-12T13:58:52Z

@siilike : thanks for your patience! I left a few comments, but apart from these small issues I think it is good to go now.

siilike · 2019-12-12T14:41:19Z

Great, thank you. I made the fixes.

jsteemann · 2019-12-12T15:15:18Z

Thanks a lot for the patch!

Add logical types. Minor improvements.

98e6178

siilike mentioned this pull request Jun 27, 2019

Logical types #35

Open

revert Buffer changes

26c170f

siilike added 2 commits September 3, 2019 11:56

updates

122ef80

Merge remote-tracking branch 'upstream/master' into siilike

e8ed9f2

updates

5885e6f

siilike force-pushed the siilike branch from b5b9e47 to 5885e6f Compare September 3, 2019 10:46

fix indentation

29cef2d

siilike force-pushed the siilike branch from 35ed139 to c61fb84 Compare September 3, 2019 11:46

minor fixes and updates

c6b9b74

siilike force-pushed the siilike branch from c61fb84 to c6b9b74 Compare September 3, 2019 11:54

jsteemann suggested changes Sep 4, 2019

View reviewed changes

Apply suggestions from code review

bf91354

Fix first issues Co-Authored-By: Jan <jsteemann@users.noreply.github.com>

jsteemann suggested changes Sep 10, 2019

View reviewed changes

third take on tags

fae2814

siilike force-pushed the siilike branch from 8c02982 to fae2814 Compare September 15, 2019 08:41

add ValueSlice tests and fixes

a26d9a7

siilike added 2 commits November 10, 2019 17:27

remove ValueSlice

5d6f711

Merge remote-tracking branch 'upstream/master' into siilike

a3756f3

make xxHash use vmalloc & vfree

bf5137d

siilike force-pushed the siilike branch from 19337e3 to bf5137d Compare November 10, 2019 15:46

siilike added 2 commits November 17, 2019 22:09

more fixes and improvements

2e201be

fix memory management override

aeb673e

siilike force-pushed the siilike branch from 0e8bdca to aeb673e Compare November 18, 2019 14:51

make thread_locals optional

6cd9b16

siilike force-pushed the siilike branch from 29e42ac to 6cd9b16 Compare November 18, 2019 20:00

fix disabling threadlocals

ee220ba

siilike mentioned this pull request Nov 19, 2019

Add tags support arangodb/java-velocypack#15

Merged

change BCD exponent to signed

858468b

jsteemann reviewed Dec 12, 2019

View reviewed changes

src/Dumper.cpp Outdated Show resolved Hide resolved

include/velocypack/velocypack-memory.h Outdated Show resolved Hide resolved

src/Builder.cpp Outdated Show resolved Hide resolved

add velocypack prefixes and fix dumping timestamps

ac70e54

jsteemann approved these changes Dec 12, 2019

View reviewed changes

jsteemann merged commit 413e695 into arangodb:master Dec 12, 2019


		static bool contains(Slice const& slice, Slice const& other);

		static bool containsObject(Slice const& slice, Slice const& other);

	return type(valueHead());
	return SliceStaticData::TypeMap[head()];

Add logical types. Minor improvements. #34

Add logical types. Minor improvements. #34

Uh oh!

Conversation

siilike commented Jun 24, 2019

graetzer commented Jun 27, 2019

graetzer commented Jun 27, 2019

siilike commented Jun 27, 2019

siilike commented Jul 31, 2019

jsteemann commented Sep 3, 2019

siilike commented Sep 3, 2019

siilike commented Sep 3, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsteemann commented Sep 4, 2019

jsteemann commented Sep 4, 2019

jsteemann commented Sep 4, 2019

siilike commented Sep 4, 2019

siilike commented Sep 4, 2019

siilike commented Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siilike commented Sep 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

siilike commented Oct 11, 2019

coveralls commented Nov 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

siilike commented Nov 10, 2019

OmarAyo commented Nov 11, 2019

OmarAyo commented Nov 15, 2019

siilike commented Nov 21, 2019

siilike commented Nov 28, 2019

siilike commented Dec 9, 2019

jsteemann commented Dec 9, 2019

siilike commented Dec 9, 2019

Uh oh!

Uh oh!

Uh oh!

jsteemann commented Dec 12, 2019

siilike commented Dec 12, 2019

jsteemann commented Dec 12, 2019

Labels

5 participants

siilike commented Sep 15, 2019 •

edited

Loading

coveralls commented Nov 10, 2019 •

edited

Loading