CNDB-14997: Fix column selection on SAI-ordered queries #1944
Conversation
Checklist before you submit for review
|
adelapena
force-pushed
the
CNDB-14997-main
branch
from
9f5a4f7 to
63a6636
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes column selection behavior in SAI-ordered queries by ensuring that column information is properly preserved when creating PK-sorted in-memory row iterators. The TopKProcessor was incorrectly returning base table columns instead of the actual column selection from the original iterator.
- Modified
PartitionInfoto include and preserve column selection information - Updated
InMemoryUnfilteredPartitionIteratorto use preserved columns instead of base table columns - Added comprehensive tests for column selection functionality
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/java/org/apache/cassandra/index/sai/utils/PartitionInfo.java | Enhanced to store and preserve column selection from original iterators |
| src/java/org/apache/cassandra/index/sai/utils/InMemoryUnfilteredPartitionIterator.java | Fixed to use preserved columns instead of base table columns |
| test/unit/org/apache/cassandra/index/sai/utils/PartitionInfoTest.java | Added unit tests for PartitionInfo functionality |
| test/unit/org/apache/cassandra/index/sai/cql/ColumnSelectionTest.java | Added integration tests for column selection with SAI indexes |
| src/java/org/apache/cassandra/db/Columns.java | Minor comment formatting change |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| public void serializeSubset(Collection<ColumnMetadata> columns, Columns superset, DataOutputPlus out) throws IOException | ||
| { | ||
| /** | ||
| /* |
There was a problem hiding this comment.
This change from '/**' to '/*' converts a Javadoc comment to a regular block comment, which removes it from API documentation. This appears unrelated to the column selection fix and may be unintentional.
| /* | |
| /** |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I had the same comment in my draft review :P
There was a problem hiding this comment.
I guess Copilot didn't consider the issue description. The bug, albeit produced somewhere else, affects the logic described here, and the trace of the exception points to this explanation. So I think it's related.
There was a problem hiding this comment.
Looks good to me. Sonar did not finish on the last CI run. Happened to me before. Maybe when you rebase and push it will finish. Until then, even with my approval, the PR cannot be merged. testOverwriteWithTTL failure seems to me as unrelated to this patch. But we would want to open a ticket for it.
| public void serializeSubset(Collection<ColumnMetadata> columns, Columns superset, DataOutputPlus out) throws IOException | ||
| { | ||
| /** | ||
| /* |
| public void serializeSubset(Collection<ColumnMetadata> columns, Columns superset, DataOutputPlus out) throws IOException | ||
| { | ||
| /** | ||
| /* |
There was a problem hiding this comment.
I had the same comment in my draft review :P
| public class ColumnSelectionTest extends VectorTester | ||
| { | ||
| /** | ||
| * Tests that we can select any column in any table with SAI indexes. See CNDB-14997 for further details. |
There was a problem hiding this comment.
| * Tests that we can select any column in any table with SAI indexes. See CNDB-14997 for further details. | |
| * Tests that we can select any type of column in any table with SAI indexes. See CNDB-14997 for further details. |
|
You may want to fill also the PR checklist |
|
I'm removing the |
When SAI's TopKProcessor created PK-sorted in-memory row iterators, it wasn't preserving the actual column information from the base row iterator. Instead, it was returning the columns of the base table, leading to incorrect column selection behavior. The fix consists on copying the column selection of the original iterator into PartitionInfo, so the PK-sorted iterator can return that same column selection.
adelapena
force-pushed
the
CNDB-14997-main
branch
from
beb219d to
2968e09
Compare
|
❌ Build ds-cassandra-pr-gate/PR-1944 rejected by Butler1 regressions found Found 1 new test failures
Found 1 known test failures |
When SAI's TopKProcessor created PK-sorted in-memory row iterators, it wasn't preserving the actual column information from the base row iterator. Instead, it was returning the columns of the base table, leading to incorrect column selection behavior.
The fix consists on copying the column selection of the original iterator into PartitionInfo, so the PK-sorted iterator can return that same column selection.