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

prepared_statement: fix deserialization of compound partition keys #795

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

piodul
Copy link
Collaborator

@piodul piodul commented Aug 22, 2023

When PartitionKeyDeserializer wants to deserialize i-th column of the partition key, it needs to know the type of that column. It has access to PreparedMetadata of the prepared query the partition key was constructed from, which has a mapping between bind markers and types. Hence, it needs to learn to which bind marker the i-th pk column corresponds to.

The self.pk_indexes of type Vec<PartitionKeyIndex> is supposed to help with that. The PartitionKeyIndex structure looks like this:

pub struct PartitionKeyIndex {
    /// index in the serialized values
    pub index: u16,
    /// sequence number in partition key
    pub sequence: u16,
}

So, we need to find an entry pki with pki.sequence == i and pki.index will be the desired bind marker index.

Currently, we naively look at the self.pk_indexes[i] entry - which is wrong. The pk_indexes vec is actually ordered by index, so we might accidentally obtain an index of the wrong partition key column this way. Although there was a test that was supposed to check this scenario, it constructed PreparedMetadata manually and didn't sort the pk_indexes by index field, which resulted in it accidentally passing.

In order to fix this, the PartitionKeyIndex is now doing a linear lookup in order to find a pki with pki.sequence == i. This lookup should be fine as partition keys usually have a little number of fields.

Fixes: #793

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

When PartitionKeyDeserializer wants to deserialize i-th column of the
partition key, it needs to know the type of that column. It has access
to PreparedMetadata of the prepared query the partition key was
constructed from, which has a mapping between bind markers and types.
Hence, it needs to learn to which bind marker the i-th pk column
corresponds to.

The `self.pk_indexes` of type `Vec<PartitionKeyIndex>` is supposed to
help with that. The PartitionKeyIndex structure looks like this:

    pub struct PartitionKeyIndex {
        /// index in the serialized values
        pub index: u16,
        /// sequence number in partition key
        pub sequence: u16,
    }

So, we need to find an entry `pki` with `pki.sequence == i` and
`pki.index` will be the desired bind marker index.

Currently, we naively look at the `self.pk_indexes[i]` entry - which is
wrong. The `pk_indexes` vec is actually ordered by `index`, so we might
accidentally obtain an index of the wrong partition key column this way.
Although there was a test that was supposed to check this scenario, it
constructed `PreparedMetadata` manually and didn't sort the `pk_indexes`
by `index` field, which resulted in it accidentally passing.

In order to fix this, the `PartitionKeyIndex` is now doing a linear
lookup in order to find a `pki` with `pki.sequence == i`. This lookup
should be fine as partition keys usually have a little number of fields.

Fixes: scylladb#793
@piodul piodul requested a review from wprzytula August 22, 2023 06:59
@wprzytula wprzytula merged commit ffd374b into scylladb:main Aug 22, 2023
8 checks passed
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.

Partition key might not be correctly decoded when putting it to tracing
2 participants