CNDB-15485: Fix PrimaryKeyWithSource comparisons #2027
Conversation
We should not take the quick path and compare rowIds between keys with no clustering, because rowIds may differ for the keys within the same partition.
Checklist before you submit for review
|
| // This optimisation is valid only when both primary keys have clustering components. | ||
| // We must not compare by rowId when the clustering is empty, because two keys | ||
| // from the same partition may have different rowIds, yet they should be considered equal in that case. | ||
| if (o instanceof PrimaryKeyWithSource && !hasEmptyClustering() && !o.hasEmptyClustering()) |
There was a problem hiding this comment.
do we have unit tests about this class PrimaryKeyWithSource ?
if we have no tests I think that we should add them, so that we can seal this behavior and prevent regressions
There was a problem hiding this comment.
Good idea to add them, yes, I'll do
|
❌ Build ds-cassandra-pr-gate/PR-2027 rejected by Butler5 regressions found Found 5 new test failures
No known test failures found |
|
Putting this on hold, as apparently there are some parts of the code that rely on ordering of |
| // This optimisation is valid only when both primary keys have clustering components. | ||
| // We must not compare by rowId when the clustering is empty, because two keys | ||
| // from the same partition may have different rowIds, yet they should be considered equal in that case. | ||
| if (o instanceof PrimaryKeyWithSource && !hasEmptyClustering()) |
There was a problem hiding this comment.
We need a more nuanced implementation. The point of PrimaryKeyWithSource is to avoid calling loadDeferred(), but this does just that. I think we should consider moving PrimaryKeyWithSource logic into individual PrimaryKeyMap implementations, and then we can do checks on the schema when we load the map, not on each key. For example, we would take the slow path if there is a static column and the fast path otherwise. This also makes it easier to differentiate between partition aware and row aware keys.
5 participants
We should not take the quick path and compare rowIds
between keys with no clustering, because rowIds may
differ for the keys within the same partition.