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.

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

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

IndexOutOfBoundsException when accessing partition where the column was deleted #3731

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a14b820
Fixing IndexOutOfBoundsException for deleted column.
sunil9977 Dec 6, 2024
34d15b9
Adding a method to check valid cell value.
sunil9977 Dec 12, 2024
a840126
Moving getValidCellBuffer method to Cell for reusability.
sunil9977 Dec 12, 2024
27c9a81
Removing the use of nowInSeonds() and implemeted patch of David.
sunil9977 Dec 24, 2024
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
6 changes: 3 additions & 3 deletions src/java/org/apache/cassandra/db/filter/RowFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ public boolean isSatisfiedBy(TableMetadata metadata, DecoratedKey partitionKey,
if (column.type.isCounter())
{
ByteBuffer foundValue = getValue(metadata, partitionKey, row);
if (foundValue == null)
if (foundValue == null || foundValue.remaining() == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

this patch is missing org.apache.cassandra.db.filter.RowFilter.MapElementExpression#isSatisfiedBy as well.

Rather than updating every code path, why not push this logic into getValue?

In getValue we can do

default:
                    Cell<?> cell = row.getCell(column);
                    if (cell == null) return null;
                    ByteBuffer bb = cell.buffer();
                    return bb.hasRemaining() ? bb : null;

with that one change all 4 code paths impacted are now seeing null (which they already handle)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am also curious why we have empty bytes rather than null in this case... this implies that the pattern of

Cell<?> cell = row.getCell(column);
return cell == null ? null : cell.buffer();

is unsafe and has always been wrong... yet this pattern is common...

$ grep -r 'cell == null ? null : cell.buffer()' src/
src//java/org/apache/cassandra/db/marshal/MapType.java:            return cell == null ? null : cell.buffer();
src//java/org/apache/cassandra/db/marshal/ListType.java:            return cell == null ? null : cell.buffer();
src//java/org/apache/cassandra/db/marshal/UserType.java:            return cell == null ? null : cell.buffer();
src//java/org/apache/cassandra/db/filter/RowFilter.java:                    return cell == null ? null : cell.buffer();
src//java/org/apache/cassandra/index/internal/CassandraIndex.java:                               cell == null ? null : cell.buffer()

Copy link
Contributor

Choose a reason for hiding this comment

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

looking at callers of org.apache.cassandra.db.rows.Cell#buffer there are 62+ code paths that are similar pattern...

Copy link
Contributor

Choose a reason for hiding this comment

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

looked into it... we do org.apache.cassandra.cql3.terms.Constants.Deleter when we delete a column. This is defined as

builder.addCell(BufferCell.tombstone(column, timestamp, nowInSec, path));

which is

public static BufferCell tombstone(ColumnMetadata column, long timestamp, long nowInSec, CellPath path)
{
    return new BufferCell(column, timestamp, NO_TTL, nowInSec, ByteBufferUtil.EMPTY_BYTE_BUFFER, path);
}

so delete column is defined as write empty bytes... that explains the empty bytes at least

return false;

ByteBuffer counterValue = LongType.instance.decompose(CounterContext.instance().total(foundValue, ByteBufferAccessor.instance));
Expand All @@ -715,7 +715,7 @@ public boolean isSatisfiedBy(TableMetadata metadata, DecoratedKey partitionKey,
{
// Note that CQL expression are always of the form 'x < 4', i.e. the tested value is on the left.
ByteBuffer foundValue = getValue(metadata, partitionKey, row);
return foundValue != null && operator.isSatisfiedBy(column.type, foundValue, value);
return foundValue != null && foundValue.remaining() > 0 && operator.isSatisfiedBy(column.type, foundValue, value);
}
}
else if (operator.appliesToCollectionElements() || operator.appliesToMapKeys())
Expand All @@ -730,7 +730,7 @@ else if (operator.appliesToCollectionElements() || operator.appliesToMapKeys())
else
{
ByteBuffer foundValue = getValue(metadata, partitionKey, row);
return foundValue != null && operator.isSatisfiedBy(column.type, foundValue, value);
return foundValue != null && foundValue.remaining() > 0 && operator.isSatisfiedBy(column.type, foundValue, value);
}
}
throw new AssertionError();
Expand Down
18 changes: 18 additions & 0 deletions test/unit/org/apache/cassandra/cql3/validation/operations/SelectTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2554,6 +2554,24 @@ 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));
});
}

@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