cqltypes: Serialize None values in collections as NULLs #241
Conversation
|
Look good, while we are waiting for the ccm to be fixed. Maybe you should open a PR to upstream as well ? |
cassandra/cqltypes.py
Outdated
| buf.write(keybytes) | ||
| buf.write(pack(len(valbytes))) | ||
| buf.write(valbytes) | ||
| if keybytes is not None: |
There was a problem hiding this comment.
Nitpick: shouldn't you check if key is None, instead of first converting it to binary and checking if the result is None?
|
Also, you should probably have a test for this change, but I don't how the driver is tested (is it tested against some real version of Cassandra/Scylla?). |
Lorak-mmk
force-pushed
the
fix-none-in-collections
branch
from
11f79e6 to
3851c1a
Compare
|
Applied the change suggested by @nyh , also added an integration tests to verify new bahavior. |
cassandra/cqltypes.py
Outdated
| buf.write(pack(len(itembytes))) | ||
| buf.write(itembytes) | ||
| if item is None: | ||
| buf.write(int32_pack(-1)) |
There was a problem hiding this comment.
I think this (and also two more times below) should be pack instead of int32_pack.
I don't think we actually support of test protocol version < 3, but if we use int16_pack in pack(len(keybytes)), I don't see any reason why packing -1 should be different.
I don't think this matters in practice because nobody uses cql version < 3, but let's make this clean.
There was a problem hiding this comment.
Before my changes there were 2 places that used int32_pack(-1) - serializing tuples and UDTs. Now that I think of it, it may be because those are not supported before v3?
More importantly, is the pack logic (pack = int32_pack if protocol_version >= 3 else uint16_pack) sound? Before making this change I looked at docs for v2: https://github.com/apache/cassandra/blob/c08aaf69b4bb62e2fe50342dfaa329dfd833ee1e/doc/native_protocol_v2.spec
Section 6 says:
All values are represented as [bytes] in EXECUTE and RESULT messages.
The [bytes] format includes an int prefix denoting the length of the value.
For that reason, the serialization formats described here will not include
a length component.
For legacy compatibility reasons, note that most non-string types support
"empty" values (i.e. a value with zero length). An empty value is distinct
from NULL, which is encoded with a negative length.
[bytes] has 4-byte length field. I think there may be something I don't understand, but I don't see how pack is correct.
There was a problem hiding this comment.
I have no idea if this is correct for version 2 of the protocol, but it can be tested (on an antique version of Cassandra). What bothered me was the inconsistency - one branch of the if() uses "pack" and the other "int32_pack", and that must be wrong (I don't know which branch is more wrong...).
There was a problem hiding this comment.
Ok, the current code is the correct one - I'll fix my change. Sections 6.10-6.13 of aforementioned document describe that each item of collection is serialized as [short bytes], not [bytes].
| i = 0 | ||
| for simple_type in PRIMITIVE_DATATYPES_KEYS: | ||
| if simple_type == 'blob': | ||
| continue # Unhashable type |
There was a problem hiding this comment.
What does this comment mean? You can have a list of blobs in CQL, I'm pretty sure it also works somehow in the Python driver?
Fixes scylladb#201 When using parepared statements, None values in collections were serialized as empty values (values with length == 0). This is unexpected and inconsistent - None values are serialized as NULLs (vlaues with length == -1) in other cases: - Statement arguments, both for simple and prepared statements - Collection elements in simple statement This commit fixes this weird behavior - now None values should be serialized as NULLs in all cases. It also adds an integration test that checks new behavior.
5.0 is no longer supported.
Lorak-mmk
force-pushed
the
fix-none-in-collections
branch
from
3851c1a to
4f5f800
Compare
When using parepared statements, None values in collections were serialized as empty values (values with length == 0). This is unexpected and inconsistent - None values are serialized as NULLs (vlaues with length == -1) in other cases:
This commit fixes this weird behavior - now None values should be serialized as NULLs in all cases.
Fixes #201