Skip to content

CNDB-15608 add TokenOnlyPrimaryKey class and isTokenOnly #2157

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.

Sign up for GitHub

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

Jump to bottom
Merged
k-rus merged 5 commits into from
Dec 19, 2025
Merged

CNDB-15608 add TokenOnlyPrimaryKey class and isTokenOnly #2157

k-rus merged 5 commits into from
Dec 19, 2025

Conversation

@k-rus
Copy link
Member

@k-rus k-rus commented Dec 8, 2025 •
edited
Loading

What is the issue

Part of https://github.com/riptano/cndb/issues/15608

The patch in CNDB-15608 will remove token from the partition key serialization, thus the case, when only token is provided in primary key, needs to be treated separately. Currently only token primary key case is blurred with complete or deferred primary keys.

What does this PR fix and why was it fixed

It implements clear separation for primary keys with token only by introducing own TokenOnlyPrimaryKey class.
It also implement isOnlyToken() method, which is used instead of comparing partition key with null in appropriate cases (e.g., deferred partition key was loaded). New tests are added to exercise TokenOnlyPrimaryKey more and improve code coverage.

CNDB's PR: https://github.com/riptano/cndb/pull/16258

@github-actions
Copy link

github-actions bot commented Dec 8, 2025 •
edited by k-rus
Loading

Checklist before you submit for review

  • This PR adheres to the Definition of Done
  • Make sure there is a PR in the CNDB project updating the Converged Cassandra version
  • Use NoSpamLogger for log lines that may appear frequently in the logs
  • Verify test results on Butler
  • Test coverage for new/modified code is > 80%
  • Proper code formatting
  • Proper title for each commit staring with the project-issue number, like CNDB-1234
  • Each commit has a meaningful description
  • Each commit is not very long and contains related changes
  • Renames, moves and reformatting are in distinct commits
  • All new files should contain the DataStax copyright header instead of the Apache License one

@k-rus k-rus force-pushed the rf-15608-token-only branch 3 times, most recently from 740185a to 914db36 Compare December 9, 2025 10:15
@k-rus k-rus changed the title [WIP] CNDB-15608 add TokenOnlyPrimaryKey class and isTokenOnly CNDB-15608 add TokenOnlyPrimaryKey class and isTokenOnly Dec 10, 2025
@k-rus
Copy link
Member Author

k-rus commented Dec 10, 2025

3 regressions found
See build details here

No new regressions or failures related to this PR.

@k-rus k-rus force-pushed the rf-15608-token-only branch 2 times, most recently from 722955d to f2603c2 Compare December 10, 2025 21:27
@k-rus
Copy link
Member Author

k-rus commented Dec 11, 2025

2 regressions found
See build details here

No new failures

eolivelli
eolivelli requested changes Dec 11, 2025
View reviewed changes
@eolivelli eolivelli requested a review from pkolaczk December 11, 2025 10:54
pkolaczk
pkolaczk requested changes Dec 12, 2025
View reviewed changes
Copy link

@pkolaczk pkolaczk left a comment

Choose a reason for hiding this comment

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

Requesting changes because some assertion messages should be definitely improved.
Besides that the code looks good and I had some minor nits / suggestions on further hardening it. This is up for discussion whether we want to do it here, or open followup issues.

@k-rus k-rus requested review from eolivelli and pkolaczk December 15, 2025 10:47
pkolaczk
pkolaczk approved these changes Dec 15, 2025
View reviewed changes
Copy link

@pkolaczk pkolaczk left a comment

Choose a reason for hiding this comment

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

Looks good. Found just two minor nits (javadoc and code formatting).
No rereview needed from my side.

eolivelli
eolivelli approved these changes Dec 15, 2025
View reviewed changes
Copy link

@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.

+1

@k-rus
Copy link
Member Author

k-rus commented Dec 15, 2025

o.a.c.index.sai.disk.v2.RowAwarePrimaryKeyTest.testComparisonBetweenTokenOnlyKeysWithDifferentTokens (compression)

This failure was already fixed, so this is false positive from Butler.

@k-rus
Copy link
Member Author

k-rus commented Dec 15, 2025

5 regressions found See build details here

One already fixed test and the rest is not relevant.

@k-rus k-rus force-pushed the rf-15608-token-only branch from ca2efed to 54964d5 Compare December 16, 2025 15:20
@k-rus
Copy link
Member Author

k-rus commented Dec 18, 2025

Merging the PR is blocked by #2157

k-rus added 5 commits December 19, 2025 15:18
@k-rus
Add TokenOnlyPrimaryKey class and replace isTokenOnly
e29b20f 
It's part of the patch.
@k-rus
Improve and fix changes
88c5304 
Addressing reviewers' comments
- Be explicit about not thread safe
- Improve assertion messages
- Fix bug in primary key comparison
- Make TokenOnlyPrimaryKey final and fix visibilities of members
- Fix formatting
- Fix RAM usage for TokenOnlyPrimaryKey
- Fix to string conversion for TokenOnlyPrimaryKey
@k-rus
Implement test of comparing TokenOnly with row aware primary keys
f5c2c96 
This removes need in a confusing assertion in another test for code
coverage.

Fixes explicit typing in the affected test file.
@k-rus
Revert incorrect formatting
0d57c4f
@k-rus
Fix formatting, incorrect test assertion, javadoc
600ba8d
@k-rus k-rus force-pushed the rf-15608-token-only branch from 54964d5 to 600ba8d Compare December 19, 2025 14:18
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 19, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
82.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@cassci-bot
Copy link

cassci-bot commented Dec 19, 2025

❌ Build ds-cassandra-pr-gate/PR-2157 rejected by Butler

2 regressions found
See build details here

Found 2 new test failures

Test Explanation Runs Upstream
o.a.c.index.sai.cql.VectorCompaction100dTest.testZeroOrOneToManyCompaction[db true] NEW 🔴⚪ 0 / 19
o.a.c.index.sai.cql.VectorSiftSmallTest.testCompaction[ca false] (compression) REGRESSION 🔵🔴 0 / 19

Found 1 known test failures

@k-rus k-rus merged commit ac7d5b6 into main Dec 19, 2025
484 of 496 checks passed
@k-rus k-rus deleted the rf-15608-token-only branch December 19, 2025 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pkolaczk pkolaczk pkolaczk approved these changes

@eolivelli eolivelli eolivelli approved these changes

@michaeljmarshall michaeljmarshall michaeljmarshall approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@k-rus @cassci-bot @pkolaczk @eolivelli @michaeljmarshall