bahman/scylla
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

materialized views: allow empty strings in views and indexes

Browse Source 
Although Cassandra generally does not allow empty strings as partition
keys (note they are allowed as clustering keys!), it *does* allow empty
strings in regular columns to be indexed by a secondary index, or to
become an empty partition-key column in a materialized view. As noted in
issues #9375 and #9364 and verified in a few xfailing cql-pytest tests,
Scylla didn't allow these cases - and this patch fixes that.

The patch mostly *removes* unnecessary code: In one place, code
prevented an sstable with an empty partition key from being written.
Another piece of removed code was a function is_partition_key_empty()
which the materialized-view code used to check whether the view's
row will end up with an empty partition key, which was supposedly
forbidden. But in fact, should have been allowed like they are allowed
in Cassandra and required for the secondary-index implementation, and
the entire function wasn't necessary.

Note that the removed function is_partition_key_empty() was *NOT* required
for the "IS NOT NULL" feature of materialized views - this continues to
work as expected after this patch, and we add another test to confirm it.
Being null and being an empty string are two different things.

This patch also removes a part of a unit test which enshrined the
wrong behavior.

After this patch we are left with one interesting difference from
Cassandra: Though Cassandra allows a user to create a view row with an
empty-string partition key, and this row is fully visible in when
scanning the view, this row can *not* be queried individually because
"WHERE v=''" is forbidden when v is the partition key (of the view).
Scylla does not reproduce this anomaly - and such point query does work
in Scylla after this patch. We add a new test to check this case, and mark
it "cassandra_bug", i.e., it's a Cassandra behavior which we consider
wrong and don't want to emulate.

This patch relies on #9352 and #10178 having been fixed in previous patches,
otherwise the WHERE v='' does not work when reading from sstables.
We add to the already existing tests we had for empty materialized-views
keys a lookup with WHERE v='' which failed before fixing those two issues.

Fixes #9364
Fixes #9375

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This commit is contained in:
Nadav Har'El
2021-09-22 17:57:20 +03:00
parent bc4d0fd5ad
commit ef43531fb6
5 changed files with 85 additions and 92 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
db/view/view.cc
View File

@@ -283,32 +283,6 @@ static bool update_requires_read_before_write(const schema& base,
return false;
}
static bool is_partition_key_empty(
const schema& base,
const schema& view_schema,
const partition_key& base_key,
const clustering_row& update) {
// Empty partition keys are not supported on normal tables - they cannot
// be inserted or queried, so enforce those rules here.
if (view_schema.partition_key_columns().size() > 1) {
// Composite partition keys are different: all components
// are then allowed to be empty.
return false;
}
auto* base_col = base.get_column_definition(view_schema.partition_key_columns().front().name());
switch (base_col->kind) {
case column_kind::partition_key:
return base_key.get_component(base, base_col->position()).empty();
case column_kind::clustering_key:
return update.key().get_component(base, base_col->position()).empty();
default:
// No multi-cell columns in the view's partition key
auto& c = update.cells().cell_at(base_col->id);
atomic_cell_view col_value = c.as_atomic_cell(*base_col);
return !col_value.is_live() || col_value.value().empty();
}
}
// Checks if the result matches the provided view filter.
// It's currently assumed that the result consists of just a single row.
class view_filter_checking_visitor {
@@ -666,7 +640,7 @@ static void add_cells_to_view(const schema& base, const schema& view, row base_c
* This method checks that the base row does match the view filter before applying anything.
*/
void view_updates::create_entry(const partition_key& base_key, const clustering_row& update, gc_clock::time_point now) {
if (is_partition_key_empty(*_base, *_view, base_key, update) || !matches_view_filter(*_base, _view_info, base_key, update, now)) {
if (!matches_view_filter(*_base, _view_info, base_key, update, now)) {
return;
}
deletable_row& r = get_view_row(base_key, update);
@@ -684,7 +658,7 @@ void view_updates::create_entry(const partition_key& base_key, const clustering_
void view_updates::delete_old_entry(const partition_key& base_key, const clustering_row& existing, const clustering_row& update, gc_clock::time_point now) {
// Before deleting an old entry, make sure it was matching the view filter
// (otherwise there is nothing to delete)
if (!is_partition_key_empty(*_base, *_view, base_key, existing) && matches_view_filter(*_base, _view_info, base_key, existing, now)) {
if (matches_view_filter(*_base, _view_info, base_key, existing, now)) {
do_delete_old_entry(base_key, existing, update, now);
}
}
@@ -795,11 +769,11 @@ bool view_updates::can_skip_view_updates(const clustering_row& update, const clu
void view_updates::update_entry(const partition_key& base_key, const clustering_row& update, const clustering_row& existing, gc_clock::time_point now) {
// While we know update and existing correspond to the same view entry,
// they may not match the view filter.
if (is_partition_key_empty(*_base, *_view, base_key, existing) || !matches_view_filter(*_base, _view_info, base_key, existing, now)) {
if (!matches_view_filter(*_base, _view_info, base_key, existing, now)) {
create_entry(base_key, update, now);
return;
}
if (is_partition_key_empty(*_base, *_view, base_key, update) || !matches_view_filter(*_base, _view_info, base_key, update, now)) {
if (!matches_view_filter(*_base, _view_info, base_key, update, now)) {
do_delete_old_entry(base_key, existing, update, now);
return;
}

3
sstables/sstables.cc
View File

@@ -2345,9 +2345,6 @@ void sstable::set_first_and_last_keys() {
return;
}
auto decorate_key = [this] (const char *m, const bytes& value) {
if (value.empty()) {
throw malformed_sstable_exception(format("{} key of summary of {} is empty", m, get_filename()));
}
auto pk = key::from_bytes(value).to_partition_key(*_schema);
return dht::decorate_key(*_schema, std::move(pk));
};

50
test/boost/view_schema_pkey_test.cc
View File

@@ -714,55 +714,5 @@ SEASTAR_TEST_CASE(test_base_non_pk_columns_in_view_partition_key_are_non_emtpy)
{utf8_type->decompose(data_value(""))},
});
}
auto views_not_matching = {
"create materialized view {} as select * from cf "
"where p1 is not null and p2 is not null and c is not null and v is not null "
"primary key (c, p1, p2, v)",
"create materialized view {} as select * from cf "
"where p1 is not null and p2 is not null and c is not null and v is not null "
"primary key (p2, p1, c, v)"
};
for (auto&& view : views_not_matching) {
auto name = make_view_name();
auto f = e.local_view_builder().wait_until_built("ks", name);
e.execute_cql(fmt::format(fmt::runtime(view), name)).get();
f.get();
auto msg = e.execute_cql(format("select p1, p2, c, v from {}", name)).get0();
assert_that(msg).is_rows().is_empty();
}
auto name = make_view_name();
auto f = e.local_view_builder().wait_until_built("ks", name);
e.execute_cql(fmt::format("create materialized view {} as select * from cf "
"where p1 is not null and p2 is not null and c is not null and v is not null "
"primary key (v, p1, p2, c)", name)).get();
f.get();
auto msg = e.execute_cql(format("select p1, p2, c, v from {}", name)).get0();
assert_that(msg).is_rows().is_empty();
e.local_db().flush_all_memtables().get();
e.execute_cql("update cf set v = 'a' where p1 = 1 and p2 = '' and c = ''").get();
eventually([&] {
auto msg = e.execute_cql(format("select p1, p2, c, v from {}", name)).get0();
assert_that(msg).is_rows()
.with_size(1)
.with_row({
{int32_type->decompose(1)},
{utf8_type->decompose(data_value(""))},
{utf8_type->decompose(data_value(""))},
{utf8_type->decompose("a")},
});
});
e.execute_cql("update cf set v = '' where p1 = 1 and p2 = '' and c = ''").get();
eventually([&] {
auto msg = e.execute_cql(format("select p1, p2, c, v from {}", name)).get0();
assert_that(msg).is_rows().is_empty();
});
e.execute_cql("delete v from cf where p1 = 1 and p2 = '' and c = ''").get();
eventually([&] {
auto msg = e.execute_cql(format("select p1, p2, c, v from {}", name)).get0();
assert_that(msg).is_rows().is_empty();
});
});
}

89
test/cql-pytest/test_materialized_view.py
View File

@@ -10,6 +10,8 @@ import pytest
from util import new_test_table, unique_name, new_materialized_view
from cassandra.protocol import InvalidRequest
import nodetool
# Test that building a view with a large value succeeds. Regression test
# for a bug where values larger than 10MB were rejected during building (#9047)
def test_build_view_with_large_row(cql, test_keyspace):
@@ -64,11 +66,10 @@ def test_mv_select_stmt_bound_values(cql, test_keyspace):
# is not allowed as a partition key. However, an empty string is a valid
# value for a string column, so if we have a materialized view with this
# string column becoming the view's partition key - the empty string may end
# up being the view row's partition key! This case should be supported,
# up being the view row's partition key. This case should be supported,
# because the "IS NOT NULL" clause in the view's declaration does not
# eliminate this row (an empty string is not considered NULL).
# This reproduces issue #9375.
@pytest.mark.xfail(reason="issue #9375")
# eliminate this row (an empty string is *not* considered NULL).
# Reproduces issue #9375.
def test_mv_empty_string_partition_key(cql, test_keyspace):
schema = 'p int, v text, primary key (p)'
with new_test_table(cql, test_keyspace, schema) as table:
@@ -81,10 +82,10 @@ def test_mv_empty_string_partition_key(cql, test_keyspace):
# The view row with the empty partition key should exist.
# In #9375, this failed in Scylla:
assert list(cql.execute(f"SELECT * FROM {mv}")) == [('', 123)]
# However, it is still impossible to select just this row,
# because Cassandra forbids an empty partition key on select
with pytest.raises(InvalidRequest, match='Key may not be empty'):
cql.execute(f"SELECT * FROM {mv} WHERE v=''")
# Verify that we can flush an sstable with just an one partition
# with an empty-string key (in the past we had a summary-file
# sanity check preventing this from working).
nodetool.flush(cql, mv)
# Reproducer for issue #9450 - when a view's key column name is a (quoted)
# keyword, writes used to fail because they generated internally broken CQL
@@ -119,3 +120,75 @@ def test_mv_quoted_column_names_build(cql, test_keyspace):
if list(cql.execute(f'SELECT * from {mv}')) == [(2, 1)]:
break
assert list(cql.execute(f'SELECT * from {mv}')) == [(2, 1)]
# The previous test (test_mv_empty_string_partition_key) verifies that a
# row with an empty-string partition key can appear in the view. This was
# checked with a full-table scan. This test is about reading this one
# view partition individually, with WHERE v=''.
# Surprisingly, Cassandra does NOT allow to SELECT this specific row
# individually - "WHERE v=''" is not allowed when v is the partition key
# (even of a view). We consider this to be a Cassandra bug - it doesn't
# make sense to allow the user to add a row and to see it in a full-table
# scan, but not to query it individually. This is why we mark this test as
# a Cassandra bug and want Scylla to pass it.
# Reproduces issue #9375 and #9352.
def test_mv_empty_string_partition_key_individual(cassandra_bug, cql, test_keyspace):
schema = 'p int, v text, primary key (p)'
with new_test_table(cql, test_keyspace, schema) as table:
with new_materialized_view(cql, table, '*', 'v, p', 'v is not null and p is not null') as mv:
# Insert a bunch of (p,v) rows. One of the v's is the empty
# string, which we would like to test, but let's insert more
# rows to make it more likely to exercise various possibilities
# of token ordering (see #9352).
rows = [[123, ''], [1, 'dog'], [2, 'cat'], [700, 'hello'], [3, 'horse']]
for row in rows:
cql.execute(f"INSERT INTO {table} (p,v) VALUES ({row[0]}, '{row[1]}')")
# Note that because cql-pytest runs on a single node, view
# updates are synchronous, and we can read the view immediately
# without retrying. In a general setup, this test would require
# retries.
# Check that we can read the individual partition with the
# empty-string key:
assert list(cql.execute(f"SELECT * FROM {mv} WHERE v=''")) == [('', 123)]
# The SELECT above works from cache. However, empty partition
# keys also used to be special-cased and be buggy when reading
# and writing sstables, so let's verify that the empty partition
# key can actually be written and read from disk, by forcing a
# memtable flush and bypassing the cache on read.
# In the past Scylla used to fail this flush because the sstable
# layer refused to write empty partition keys to the sstable:
nodetool.flush(cql, mv)
# First try a full-table scan, and then try to read the
# individual partition with the empty key:
assert set(cql.execute(f"SELECT * FROM {mv} BYPASS CACHE")) == {
(x[1], x[0]) for x in rows}
# Issue #9352 used to prevent us finding WHERE v='' here, even
# when the data is known to exist (the above full-table scan
# saw it!) and despite the fact that WHERE v='' is parsed
# correctly because we tested above it works from memtables.
assert list(cql.execute(f"SELECT * FROM {mv} WHERE v='' BYPASS CACHE")) == [('', 123)]
# Test that the "IS NOT NULL" clause in the materialized view's SELECT
# functions as expected - namely, rows which have their would-be view
# key column unset (aka null) do not get copied into the view.
def test_mv_is_not_null(cql, test_keyspace):
schema = 'p int, v text, primary key (p)'
with new_test_table(cql, test_keyspace, schema) as table:
with new_materialized_view(cql, table, '*', 'v, p', 'v is not null and p is not null') as mv:
cql.execute(f"INSERT INTO {table} (p,v) VALUES (123, 'dog')")
cql.execute(f"INSERT INTO {table} (p,v) VALUES (17, null)")
# Note that because cql-pytest runs on a single node, view
# updates are synchronous, and we can read the view immediately
# without retrying. In a general setup, this test would require
# retries.
# The row with 123 should appear in the view, but the row with
# 17 should not, because v *is* null.
assert list(cql.execute(f"SELECT * FROM {mv}")) == [('dog', 123)]
# The view row should disappear and reappear if its key is
# changed to null and back in the base table:
cql.execute(f"UPDATE {table} SET v=null WHERE p=123")
assert list(cql.execute(f"SELECT * FROM {mv}")) == []
cql.execute(f"UPDATE {table} SET v='cat' WHERE p=123")
assert list(cql.execute(f"SELECT * FROM {mv}")) == [('cat', 123)]
cql.execute(f"DELETE v FROM {table} WHERE p=123")
assert list(cql.execute(f"SELECT * FROM {mv}")) == []

1
test/cql-pytest/test_secondary_index.py
View File

@@ -276,7 +276,6 @@ def test_multi_column_with_regular_index(cql, test_keyspace):
# wrong or unusual about an empty string, and it should be supported just
# like any other string.
# Reproduces issue #9364
@pytest.mark.xfail(reason="issue #9364")
def test_index_empty_string(cql, test_keyspace):
schema = 'p int, v text, primary key (p)'
# Searching for v='' without an index (with ALLOW FILTERING), works