Skip to content

Conversation

@pkolaczk
Copy link

@pkolaczk pkolaczk commented Oct 29, 2025

CNDB-15485: Fix ResultRetriever key comparison to prevent dupes in result set (#2024)

(cherry picked from commit ada025c)

Copy of #2023, but targeting
cndb-main-release-202510 branch.

https://github.com/riptano/cndb/issues/15485

This PR fixes a bug introduced to this branch via
#1884. The bug only impacts
SAI file format aa when the index file was produced via compaction,
which is why the modified test simply adds coverage to compact the table
and hit the bug.

The bug happens when an iterator produces the same partition across two
different batch fetches from storage. These keys were not collapsed in
the key.equals(lastKey) logic because compacted indexes use a row id
per row instead of per partition, and the logic in
PrimaryKeyWithSource considers rows with different row ids to be
distinct. However, when we went to materialize a batch from storage, we
hit this code:

        ClusteringIndexFilter clusteringIndexFilter = command.clusteringIndexFilter(firstKey.partitionKey());
        if (cfs.metadata().comparator.size() == 0 || firstKey.hasEmptyClustering())
        {
            return clusteringIndexFilter;
        }
        else
        {
            nextClusterings.clear();
            for (PrimaryKey key : keys)
                nextClusterings.add(key.clustering());
            return new ClusteringIndexNamesFilter(nextClusterings, clusteringIndexFilter.isReversed());
        }

which returned clusteringIndexFilter for aa because those indexes do
not have the clustering information. Therefore, each batch fetched the
whole partition (which was subsequently filtered to the proper results),
and produced a multiplier effect where we saw batch many duplicates.

This fix works by comparing partition keys and clustering keys directly,
which is a return to the old comparison logic from before
#1884. There was actually a
discussion about this in the PR to main, but unfortunately, we missed
this case
#1883 (comment).

A more proper long term fix might be to remove the logic of creating a
PrimaryKeyWithSource for AA indexes. However, I preferred this
approach because it is essentially a revert instead of fixing forward
solution.

@github-actions
Copy link

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

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.

LGTM

…sult set (#2024)

(cherry picked from commit ada025c)

Copy of #2023, but targeting
`cndb-main-release-202510` branch.

riptano/cndb#15485

This PR fixes a bug introduced to this branch via
#1884. The bug only impacts
SAI file format `aa` when the index file was produced via compaction,
which is why the modified test simply adds coverage to compact the table
and hit the bug.

The bug happens when an iterator produces the same partition across two
different batch fetches from storage. These keys were not collapsed in
the `key.equals(lastKey)` logic because compacted indexes use a row id
per row instead of per partition, and the logic in
`PrimaryKeyWithSource` considers rows with different row ids to be
distinct. However, when we went to materialize a batch from storage, we
hit this code:

```java
        ClusteringIndexFilter clusteringIndexFilter = command.clusteringIndexFilter(firstKey.partitionKey());
        if (cfs.metadata().comparator.size() == 0 || firstKey.hasEmptyClustering())
        {
            return clusteringIndexFilter;
        }
        else
        {
            nextClusterings.clear();
            for (PrimaryKey key : keys)
                nextClusterings.add(key.clustering());
            return new ClusteringIndexNamesFilter(nextClusterings, clusteringIndexFilter.isReversed());
        }
```

which returned `clusteringIndexFilter` for `aa` because those indexes do
not have the clustering information. Therefore, each batch fetched the
whole partition (which was subsequently filtered to the proper results),
and produced a multiplier effect where we saw `batch` many duplicates.

This fix works by comparing partition keys and clustering keys directly,
which is a return to the old comparison logic from before
#1884. There was actually a
discussion about this in the PR to `main`, but unfortunately, we missed
this case
#1883 (comment).

A more proper long term fix might be to remove the logic of creating a
`PrimaryKeyWithSource` for AA indexes. However, I preferred this
approach because it is essentially a `revert` instead of fixing forward
solution.
@sonarqubecloud
Copy link

@cassci-bot
Copy link

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


1 regressions found
See build details here


Found 1 new test failures

Test Explanation Runs Upstream
o.a.c.concurrent.StageTimeMeasurementTest.executionAndQueueTimeAreCountedOnSubmitWithResult (compression) NEW 🔴🔵 0 / 0

No known test failures found

@pkolaczk pkolaczk merged commit 9e7fddc into cndb-main-release-202510 Oct 30, 2025
488 of 493 checks passed
@pkolaczk pkolaczk deleted the c15485-oct-release branch October 30, 2025 12:35
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