Aerospike 5.6 Expressions

aerospike / aerospike-client-rust

Rust client for the Aerospike database

https://www.aerospike.com/

Other

82 stars 29 forks source link

Aerospike 5.6 Expressions #100

Closed jonas32 closed 3 years ago

jonas32 commented 3 years ago

Content:

[x] New Basic Expressions
[x] New CDT Expressions
[x] Operation Read/Write Expressions
[x] Tests for new Expressions
[x] Tests for Expression Operations

Requires at least Aerospike 5.6. Dont merge before 5.6 release.

jonas32 commented 3 years ago

Tests are failing because of the Aerospike Version. Tested against 5.5.0.2-111. Some minor changes (comments, formatting) are still pending, but i think its review ready.

khaf commented 3 years ago

@jonas32 Is there a reason this branch is not merged yet?

jonas32 commented 3 years ago

I guess it was never reviewed completely. From my end its ready to go.

khaf commented 3 years ago

I'll give it a read and merge this week. Sorry for the delay, I thought there was a hold up before.

khaf commented 3 years ago

@jonas32 The Expression test for LET/DEF/VAR tests/src/exp.rs:406 is failing with PARAMETER_ERROR. There are also a few compiler warnings that I think should be taken care of. Will you please take a look?

jonas32 commented 3 years ago

I debugged the LET/DEF/VAR problem. This is what the server complains about:

Oct 22 2021 18:32:30 GMT: WARNING (exp): (exp.c:2304) build_let - error 4 illegal variable name 'x' at 0 - must begin with an alpha or underscore

I dont really get whats wrong here. Do you have any idea?

kportertx commented 3 years ago

Our msgpack based protocol has two string standards. The expectation here is a "raw string" which doesn't include the "type" byte that precedes a string. I suspect that this is the problem.

You can also put the exp logging context into detail in your aerospike.conf file, doing so will also print out a wire protocol dump of the expression - if you provide that I can help with troubleshooting.

jonas32 commented 3 years ago

Thanks for the info @kportertx! It's working now. Just out of interest, whats the reason for that second string type? I ran into that problem a few times already. @khaf now it should be ready. There are still a few compiler errors when you test with cargo clippy, but i think thats fine. They are fixed with the async PR. Fixing them again would be redundant and just cause merge conflicts.

kportertx commented 3 years ago

I believe the older msgpack standard conflated strings and binary types so an extra type byte was added to differentiate between strings and various binary types. Now we've mostly moved to the new standard where binary and strings are different but still have this byte on strings for backwards compatibility. The byte is still needed for binary types when they represent data for a Blob-bin which needs to differentiate between the various language specific blobs, generic blobs, and other types.