Skip to content

Conversation

@aymkhalil
Copy link
Contributor

What does this patch do
This patch fixes an issue when primary keys contain clustering key and a user issues a partition delete.

Take the following schema as an example:

CREATE TABLE ks1.test (
    pk text,
    c1 text,
    val text,
    PRIMARY KEY (pk, c1)

If a user issues a DELETE FROM test WHERE pk=<pk>, the agent would push an event with pk only (c1 will not exist) and then the connector will perform the read back using a faulty query (missing a projection clause) evident from debug logs:

[pool-6-thread-1] DEBUG com.datastax.oss.cdc.CassandraClient - SELECT FROM ks1.test WHERE pk=?

That will cause the C* driver throw an exception logged into the pulsar function (i.e. the connector)

com.datastax.oss.driver.api.core.servererrors.SyntaxError: line 1:7 no viable alternative at input 'FROM' (SELECT [FROM]...)

causing the connector not able to make progress and a build up in the events topic.

How does it fix the issue
There is a condition introduced in 2ec124f to compare the where condition length with the pk length (the latter being advised from the table schema, not the events topic) that falls back to the static columns projection clause if there is a length mismatch. This was introduced to handle static columns, which C* allows partition level updates for if the updated columns are static columns exclusively (also not that the events topic has no first citizen property indicting if the table change was a delete or update). If the static columns are absent, it makes sense to just return the projection clause (which indicate a delete by partition use cases with clustering keys existing, otherwise the where conditions length would match the pk length from schema).

A new test is added, that would failed with no viable alternative at input 'FROM' (SELECT [FROM]...) without the proposed fix


kafkaVersion=3.4.0
vavrVersion=0.10.3
testContainersVersion=1.16.2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't work anymore with Docker Desktop for Mac (with Apple Silicon)

@plpesvc-ds
Copy link

plpesvc-ds commented Feb 20, 2025

Build successful! ✅
Deploying draft.
Deploy successful! View draft

@aymkhalil aymkhalil requested a review from eolivelli February 20, 2025 19:28
@southwel
Copy link

Chatted with @aymkhalil about this. Change looks good and was tested well.

@plpesvc-ds
Copy link

plpesvc-ds commented Feb 20, 2025

Build successful! ✅
Deploying draft.
Deploy successful! View draft

@plpesvc-ds
Copy link

plpesvc-ds commented Feb 21, 2025

Build successful! ✅
Deploying draft.
Deploy successful! View draft

Copy link
Collaborator

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Lgtm

Great to also improve Ci

@aymkhalil aymkhalil merged commit 148c18e into master Feb 21, 2025
42 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.

5 participants