From 7fca3500752d9f5d87ff8625f430c41221d5baf5 Mon Sep 17 00:00:00 2001 From: Jan Ciolek Date: Wed, 21 Jun 2023 11:25:39 +0200 Subject: [PATCH 1/3] forward_service: fix forgetting case-sensitivity in aggregates There was a bug that caused aggregates to fail when used on column-sensitive columns. For example: ``` SELECT SUM("SomeColumn") FROM ks.table; ``` would fail, with a message saying that there is no column "somecolumn". This is because the case-sensitivity got lost on the way. For non case-sensitive column names we convert them to lowercase, but for case sensitive names we have to preserve the name as originally written. The problem was in `forward_service` - we took a column name and created a non case-sensitive `column_identifier` out of it. This converted the name to lowercase, and later such column couldn't be found. To fix it, let's make the `column_identifier` case-sensitive. It will preserve the name, without converting it to lowercase. Fixes: https://github.com/scylladb/scylladb/issues/14307 Signed-off-by: Jan Ciolek --- service/forward_service.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/service/forward_service.cc b/service/forward_service.cc index 8716b6ffe9..353d020365 100644 --- a/service/forward_service.cc +++ b/service/forward_service.cc @@ -333,7 +333,7 @@ static shared_ptr mock_selection( ) { auto name_as_expression = [] (const sstring& name) -> cql3::expr::expression { return cql3::expr::unresolved_identifier { - make_shared(name, false) + make_shared(name, true) }; }; From 854b0301be1cf8a64608823d3d689ed9041bb9e2 Mon Sep 17 00:00:00 2001 From: Jan Ciolek Date: Wed, 21 Jun 2023 11:42:55 +0200 Subject: [PATCH 2/3] cql-pytest/test_aggregate: test case-sensitive column name in aggregate There was a bug which made aggregates fail when used with case-sensitive column names. Add a test to make sure that this doesn't happen in the future. Signed-off-by: Jan Ciolek --- test/cql-pytest/test_aggregate.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/cql-pytest/test_aggregate.py b/test/cql-pytest/test_aggregate.py index 8ad32c754e..da6b1e29f5 100644 --- a/test/cql-pytest/test_aggregate.py +++ b/test/cql-pytest/test_aggregate.py @@ -18,6 +18,11 @@ def table1(cql, test_keyspace): with new_test_table(cql, test_keyspace, "p int, c int, v int, d double, dc decimal, PRIMARY KEY (p, c)") as table: yield table +@pytest.fixture(scope="module") +def table2(cql, test_keyspace): + with new_test_table(cql, test_keyspace, 'p int, c int, somecolumn int, "SomeColumn" int, "OtherColumn" int, PRIMARY KEY (p, c)') as table: + yield table + # When there is no row matching the selection, the count should be 0. # First check a "=" expression matching no row: def test_count_empty_eq(cql, table1): @@ -217,3 +222,22 @@ def test_avg_decimal_2(cql, table1, cassandra_bug): def test_reject_aggregates_in_where_clause(cql, table1): assert_invalid_message(cql, table1, 'Aggregation', f'SELECT * FROM {table1} WHERE p = sum((int)4)') + +# Reproduces #13265 +# Aggregates must handle case-sensitive column names correctly. +def test_sum_case_sensitive_column(cql, table2): + p = unique_key_int() + cql.execute(f'insert into {table2} (p, c, somecolumn, "SomeColumn", "OtherColumn") VALUES ({p}, 1, 1, 10, 100)') + cql.execute(f'insert into {table2} (p, c, somecolumn, "SomeColumn", "OtherColumn") VALUES ({p}, 2, 2, 20, 200)') + cql.execute(f'insert into {table2} (p, c, somecolumn, "SomeColumn", "OtherColumn") VALUES ({p}, 3, 3, 30, 300)') + + assert cql.execute(f'select sum(somecolumn) from {table2} where p = {p}').one()[0] == 6 + assert cql.execute(f'select sum("somecolumn") from {table2} where p = {p}').one()[0] == 6 + assert cql.execute(f'select sum("SomeColumn") from {table2} where p = {p}').one()[0] == 60 + assert cql.execute(f'select sum(SomeColumn) from {table2} where p = {p}').one()[0] == 6 + assert cql.execute(f'select sum("OtherColumn") from {table2} where p = {p}').one()[0] == 600 + + assert_invalid_message(cql, table2, 'someColumn', + f'select sum("someColumn") from {table2} where p = {p}') + assert_invalid_message(cql, table2, 'othercolumn', + f'select sum(OtherColumn) from {table2} where p = {p}') From 16c21d7252af75cd6a37117a731903447a46753e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Cio=C5=82ek?= <36861778+cvybhu@users.noreply.github.com> Date: Wed, 21 Jun 2023 13:38:00 +0000 Subject: [PATCH 3/3] service/forward_service.cc: make case-sensitivity explicit Make it explicit that the boolean argument determines case-sensitivity. It emphasizes its importance. Signed-off-by: Jan Ciolek --- service/forward_service.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/service/forward_service.cc b/service/forward_service.cc index 353d020365..938f4b72e6 100644 --- a/service/forward_service.cc +++ b/service/forward_service.cc @@ -332,8 +332,9 @@ static shared_ptr mock_selection( const std::optional& info ) { auto name_as_expression = [] (const sstring& name) -> cql3::expr::expression { + constexpr bool keep_case = true; return cql3::expr::unresolved_identifier { - make_shared(name, true) + make_shared(name, keep_case) }; };