Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PYTHON-1369 Extend driver vector support to arbitrary subtypes and fix handling of variable length types (OSS C* 5.0) #1217

Merged
merged 25 commits into from
Sep 4, 2024

Conversation

absurdfarce
Copy link
Collaborator

No description provided.

rv.append(abs(v))

rv.reverse()
return bytes(rv)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adapted from vints_pack and vints_unpack above. Those functions are different enough (built-in zig-zag encoding + tuples for incoming/outgoing data) that it seemed worthwhile to live with the code duplication here.

"""
The driver was unable to deserialize a given vector
"""
pass
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in PYTHON-1371 to indicate an attempt to decode a vector of an unsupported subtype. With the other changes in this PR this exception is now completely unnecessary.

@classmethod
def serial_size(cls):
serialized_size = cls.subtype.serial_size()
return cls.vector_size * serialized_size if serialized_size is not None else None
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here (as in many other points in this PR) we're following the example of what's done on the server side.

def _normalize_set(self, val):
if isinstance(val, set) or isinstance(val, util.SortedSet):
return frozenset([self._normalize_set(v) for v in val])
return val
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really just about making the types line up. The set codec returns a SortedSet by default and that class doesn't have all the niceties of frozenset so we convert here to simplify the equality comparison later.

@absurdfarce
Copy link
Collaborator Author

Jenkins build at this point looks good: all failures appear to be known issues, most notably PYTHON-1390 and friends.

@absurdfarce
Copy link
Collaborator Author

@SiyaoIsHiding the integration test is now complete and covers both tuples and UDTs. At this point I think I've covered everything we intended to cover so this PR should be done (plus or minus changes coming out of review). Would you mind taking another pass at this?

@@ -217,3 +219,6 @@ def cql_encode_ipaddress(self, val):
is suitable for ``inet`` type columns.
"""
return "'%s'" % val.compressed

def cql_encode_decimal(self, val):
return self.cql_encode_float(float(val))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing a bug discovered during testing. When using the drivers support for positional parameters in the execute() statement we weren't correctly expanding decimal types.

Copy link
Collaborator

@SiyaoIsHiding SiyaoIsHiding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the type mismatch error handling, looks good to me!

idx += bytes_read
rv.append(cls.subtype.deserialize(byts[idx:idx + size], protocol_version))
idx += size
return rv
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to throw an error when the length of elements in the byts does not match the vector dimension definition? Same for serialize.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually happening (at least for deserialize) with the changes above. I've also recently added support for similar tests in serliaze() (along with tests for all the cases).

lessthanorequalcass40 = unittest.skipUnless(CASSANDRA_VERSION <= Version('4.0-a'), 'Cassandra version less or equal to 4.0 required')
lessthancass40 = unittest.skipUnless(CASSANDRA_VERSION < Version('4.0-a'), 'Cassandra version less than 4.0 required')
greaterthanorequalcass40 = unittest.skipUnless(CASSANDRA_VERSION >= Version('4.0'), 'Cassandra version 4.0 or greater required')
greaterthanorequalcass50 = unittest.skipUnless(CASSANDRA_VERSION >= Version('5.0-beta'), 'Cassandra version 5.0 or greater required')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: as far as I know there isn't a branch called '5.0-beta' anymore. It's now called '5.0.0'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent here was to handle the various betas and release candidate releases as we run up to Cassandra 5.0. Once we get to an actual 5.0 release this can be easily changed.

@absurdfarce
Copy link
Collaborator Author

I believe everything from the last go-round has been addressed. @SiyaoIsHiding would you mind taking another look?

@SiyaoIsHiding
Copy link
Collaborator

Sorry that previously I missed Counter subtype. Counter is considered a fixed length type, even if BigInt is var-sized. We may want to change the CounterColumnType as follows:

class CounterColumnType(LongType):
    typename = 'counter'

    @classmethod
    def serial_size(cls):
        return None

I don't quite understand how vector<counter, 2> works though. I don't know how we can test it.

@absurdfarce absurdfarce merged commit c4a808d into master Sep 4, 2024
4 of 5 checks passed
@absurdfarce absurdfarce deleted the python1369 branch September 4, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants