Skip to content
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.

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

IndexOutOfBoundsException when accessing partition where the column was deleted #3731

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/java/org/apache/cassandra/db/filter/RowFilter.java
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
import static org.apache.cassandra.cql3.statements.RequestValidations.checkBindValueSet;
import static org.apache.cassandra.cql3.statements.RequestValidations.checkFalse;
import static org.apache.cassandra.cql3.statements.RequestValidations.checkNotNull;
import static org.apache.cassandra.utils.FBUtilities.nowInSeconds;

/**
* A filter on which rows a given query should include or exclude.
Expand Down Expand Up @@ -529,7 +530,7 @@ protected ByteBuffer getValue(TableMetadata metadata, DecoratedKey partitionKey,
return row.clustering().bufferAt(column.position());
default:
Cell<?> cell = row.getCell(column);
return cell == null ? null : cell.buffer();
return Cell.getValidCellBuffer(cell, nowInSeconds());
Copy link
Contributor

@dcapwell dcapwell Dec 23, 2024

Choose a reason for hiding this comment

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

sorry, been bad about getting back to this... im not a fan of nowInSeconds being called here as we should really do this once per query... im trying to reviewer the higher level calling code to see if there is something we can do to improve this...

Copy link
Contributor

Choose a reason for hiding this comment

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

org.apache.cassandra.db.filter.RowFilter#filter(org.apache.cassandra.schema.TableMetadata, long)

we are passed in a nowInSeconds! It looks like we can refactor the caller to pass this value in, that way we don't call nowInSeconds for every cell (this can become a problem)

Copy link
Contributor

Choose a reason for hiding this comment

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

once we plumb nowInSeconds from the caller through here, then I am +1 to this patch

}
}

Expand Down
20 changes: 20 additions & 0 deletions src/java/org/apache/cassandra/db/rows/Cell.java
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,26 @@ public static long decodeLocalDeletionTime(long localDeletionTime, int ttl, Dese
// timestamp on expiry.
}

/**
* Validates a cell's liveliness, tombstone status, and buffer contents.
*
* @param cell The cell to validate.
* @param nowInSeconds The current time in seconds.
* @return A ByteBuffer (including valid empty buffers) if valid, or null otherwise.
*/
public static ByteBuffer getValidCellBuffer(Cell<?> cell, long nowInSeconds) {
if (cell == null || cell.isTombstone()) {
return null;
}

if (!cell.isLive(nowInSeconds)) {
return null;
}

// Allow valid empty buffers
return cell.buffer();
}

/**
* The serialization format for cell is:
* [ flags ][ timestamp ][ deletion time ][ ttl ][ path size ][ path ][ value size ][ value ]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2554,6 +2554,35 @@ public void filteringOnStaticColumnTest() throws Throwable
});
}

@Test
public void filteringOnDeletedStaticColumnValue() throws Throwable {
dcapwell marked this conversation as resolved.
Show resolved Hide resolved
// Create table with int-only columns
createTable("CREATE TABLE %s (pk0 int, pk1 int, ck0 int, ck1 int, s0 tinyint static, v0 int, v1 int, PRIMARY KEY ((pk0, pk1), ck0, ck1))");

// Insert rows
execute("INSERT INTO %s (pk0, pk1, s0, ck0, ck1, v0, v1) VALUES (?, ?, ?, ?, ?, ?, ?)", 1000, 2000, (byte) 126, 100, 1, 20, 30);
execute("INSERT INTO %s (pk0, pk1, s0, ck0, ck1, v0, v1) VALUES (?, ?, ?, ?, ?, ?, ?)", 1000, 2000, (byte) 125, 200, 2, 40, 50);
execute("INSERT INTO %s (pk0, pk1, s0, ck0, ck1, v0, v1) VALUES (?, ?, ?, ?, ?, ?, ?)", 1000, 3000, (byte) 122, 300, 3, 60, 70);
execute("DELETE s0,v0,v1 FROM %s WHERE pk0=1000 AND pk1=2000 and ck0=100 and ck1=1");

beforeAndAfterFlush(() -> {
// Verify the columns are deleted
assertRows(execute("SELECT pk0, pk1, s0, ck0, ck1, v0, v1 FROM %s WHERE s0=? ALLOW FILTERING", (byte) 122),
row(1000, 3000, (byte) 122, 300, 3, 60, 70));
});

execute("DELETE v0 FROM %s WHERE pk0=1000 AND pk1=3000 AND ck0=300 AND ck1=3");

beforeAndAfterFlush(() -> {
assertRows(execute("SELECT pk0, pk1, s0, ck0, ck1, v0, v1 FROM %s WHERE s0=? ALLOW FILTERING", (byte) 122),
row(1000, 3000, (byte) 122, 300, 3, null, 70));

assertRows(execute("SELECT pk0, pk1, s0, ck0, ck1, v0, v1 FROM %s WHERE pk0=1000 AND pk1=3000 AND ck0=300 AND ck1=3"),
row(1000, 3000, (byte) 122, 300, 3, null, 70));
});

}

@Test
public void containsFilteringOnNonClusteringColumn() throws Throwable {
createTable("CREATE TABLE %s (a int, b int, c int, d list<int>, PRIMARY KEY (a, b, c))");
Expand Down