[scylladb-dev] [PATCH v1 0/3] cql3: create_view_statement: fix

[Pavel Solodovnikov]

`create_view_statement::announce_migration()` has an incorrect
check to verify that no bound variables were supplied to a
select statement of a materialized view.

This used `variable_specifications::empty()` static method,
which doesn't check the current instance for emptiness but
constructs a new empty instance instead.

The following bit of code actually checked that the pointer
to the new empty instance is not null:

if (!_variables.empty()) {
throw exceptions::invalid_request_exception(format("Cannot use query parameters in CREATE MATERIALIZED VIEW statements"));
}

Use `get_specifications().empty()` instead to fix the semantics of
the `if` statement.

This series also removes this `empty()` method, because it's
not used anymore. The corresponding non-default constructor
is also removed due to being unused.

Tests: unit(dev)

Pavel Solodovnikov (3):
cql3: make `variable_specifications::get_specifications()` return
const-ref for lvalue overload
cql3: create_view_statement: fix check for bound variables
cql3: variable_specifications: remove unused methods

cql3/statements/create_view_statement.cc | 2 +-
cql3/variable_specifications.cc | 12 ++----------
cql3/variable_specifications.hh | 9 +--------
3 files changed, 4 insertions(+), 19 deletions(-)

--
2.31.1

[Pavel Solodovnikov]

There's no point in copying the `_specs` vector by value in such
case, just return a const reference. All existing uses create
a copy either way.

Tests: unit(dev)

Signed-off-by: Pavel Solodovnikov <pa.solo...@scylladb.com>
---
cql3/variable_specifications.cc | 4 ++--
cql3/variable_specifications.hh | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/cql3/variable_specifications.cc b/cql3/variable_specifications.cc
index b8e21d1225..58d9e86a6d 100644
--- a/cql3/variable_specifications.cc
+++ b/cql3/variable_specifications.cc
@@ -57,8 +57,8 @@ size_t variable_specifications::size() const {
return _variable_names.size();
}

-std::vector<lw_shared_ptr<column_specification>> variable_specifications::get_specifications() const & {
- return std::vector<lw_shared_ptr<column_specification>>(_specs.begin(), _specs.end());
+const std::vector<lw_shared_ptr<column_specification>>& variable_specifications::get_specifications() const & {
+ return _specs;
}

std::vector<lw_shared_ptr<column_specification>> variable_specifications::get_specifications() && {
diff --git a/cql3/variable_specifications.hh b/cql3/variable_specifications.hh
index e261851e1d..e8fb3e8aec 100644
--- a/cql3/variable_specifications.hh
+++ b/cql3/variable_specifications.hh
@@ -74,7 +74,7 @@ class variable_specifications final {

size_t size() const;

- std::vector<lw_shared_ptr<column_specification>> get_specifications() const &;
+ const std::vector<lw_shared_ptr<column_specification>>& get_specifications() const &;

std::vector<lw_shared_ptr<column_specification>> get_specifications() &&;

--
2.31.1

[Pavel Solodovnikov]

The code for checking that an MV's select statement doesn't
have any bind markers uses the wrong method and always returns
`false` even when it should not.

`variable_specifications::empty()` is a misleading name because
it doesn't check if the current instance is empty, but creates
an empty instance wrapped in a `lw_shared_ptr` instead.
Thus, the code in `create_view_statement::announce_migration()`
checks that the pointer is not empty, which is always false.

Use `get_specifications().empty()` to check that the
specifications vector inside the `variable_specifications`
instance is not empty.

Tests: unit(dev)

Signed-off-by: Pavel Solodovnikov <pa.solo...@scylladb.com>
---

cql3/statements/create_view_statement.cc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cql3/statements/create_view_statement.cc b/cql3/statements/create_view_statement.cc
index 19d9b12c25..75686eaa32 100644
--- a/cql3/statements/create_view_statement.cc
+++ b/cql3/statements/create_view_statement.cc
@@ -222,7 +222,7 @@ future<shared_ptr<cql_transport::event::schema_change>> create_view_statement::a
return def;
}));

- if (!_variables.empty()) {
+ if (!_variables.get_specifications().empty()) {

throw exceptions::invalid_request_exception(format("Cannot use query parameters in CREATE MATERIALIZED VIEW statements"));
}

--
2.31.1

[Pavel Solodovnikov]

Signed-off-by: Pavel Solodovnikov <pa.solo...@scylladb.com>
---

cql3/variable_specifications.cc | 8 --------
cql3/variable_specifications.hh | 7 -------
2 files changed, 15 deletions(-)

diff --git a/cql3/variable_specifications.cc b/cql3/variable_specifications.cc
index 58d9e86a6d..af78761668 100644
--- a/cql3/variable_specifications.cc
+++ b/cql3/variable_specifications.cc
@@ -45,14 +45,6 @@

namespace cql3 {

-variable_specifications::variable_specifications(const std::vector<::shared_ptr<column_identifier>>& variable_names)
- : _variable_names{variable_names}
- , _specs{variable_names.size()}
- , _target_columns{variable_names.size()}
-{ }
-lw_shared_ptr<variable_specifications> variable_specifications::empty() {
- return make_lw_shared<variable_specifications>(std::vector<::shared_ptr<column_identifier>>{});
-}

size_t variable_specifications::size() const {
return _variable_names.size();
}

diff --git a/cql3/variable_specifications.hh b/cql3/variable_specifications.hh
index e8fb3e8aec..caf1c8bfe2 100644
--- a/cql3/variable_specifications.hh
+++ b/cql3/variable_specifications.hh
@@ -64,13 +64,6 @@ class variable_specifications final {
public:

variable_specifications() = default;
- variable_specifications(const std::vector<::shared_ptr<column_identifier>>& variable_names);
-
- /**
- * Returns an empty instance of <code>VariableSpecifications</code>.
- * @return an empty instance of <code>VariableSpecifications</code>
- */
- static lw_shared_ptr<variable_specifications> empty();

size_t size() const;

--
2.31.1

[Pavel Solodovnikov]

The topic should be: [scylladb-dev] [PATCH v1 0/3] cql3:
create_view_statement: fix wrong check for bound variables
Not sure why the rest was cut down, though :(

[Nadav Har'El]

If you want the cover letter to be included in the merged series, please provide a git URL to merge, or just create a github pull request.

--
Nadav Har'El

n...@scylladb.com

--
You received this message because you are subscribed to the Google Groups "ScyllaDB development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scylladb-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/scylladb-dev/CA%2BZQzO6dswWDs0e1pZyvNws7DFjXmHad6YUaYPGBEx%2BHb4G2FA%40mail.gmail.com.

[Nadav Har'El]

Please add a regression test for this. I'm guessing that you checked it manually somehow, trying some CQL command and seeing if it's accepted or not accepted. Well, you can do the same thing in an automated test.

I personally recommend test/cql-pytest which allows you to also check the same command against Cassandra if you're curious how it behaves for the same request. Let me know if you need help to get started with cql-pytest. Or, you can choose one of our other unit-test frameworks (the textual test/cql, or C++ unit tests in test/boost et al.)/

--
Nadav Har'El

n...@scylladb.com

On Wed, Jul 28, 2021 at 11:56 PM Pavel Solodovnikov <pa.solo...@scylladb.com> wrote:

--
You received this message because you are subscribed to the Google Groups "ScyllaDB development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scylladb-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/scylladb-dev/20210728205601.1395160-3-pa.solodovnikov%40scylladb.com.

[Nadav Har'El]

Looks good, thanks. I just had a comment that I'd like you to please add a regression test for this problem. I'm guessing that you tested it manually, so it could have been an automated test instead. Running all the existing unit tests (like you mentioned you did) is good in the sense that you didn't break any already working features - but it doesn't demonstrate that your patch really fixed the bug (or that the bug ever existed) - a new test can easily demonstrate that.

--
Nadav Har'El

n...@scylladb.com

On Wed, Jul 28, 2021 at 11:56 PM Pavel Solodovnikov <pa.solo...@scylladb.com> wrote:

--
You received this message because you are subscribed to the Google Groups "ScyllaDB development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scylladb-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/scylladb-dev/20210728205601.1395160-1-pa.solodovnikov%40scylladb.com.

[Pavel Solodovnikov]

On Thu, Jul 29, 2021 at 6:00 PM Nadav Har'El <n...@scylladb.com> wrote:
>
> Please add a regression test for this. I'm guessing that you checked it manually somehow, trying some CQL command and seeing if it's accepted or not accepted. Well, you can do the same thing in an automated test.
>
> I personally recommend test/cql-pytest which allows you to also check the same command against Cassandra if you're curious how it behaves for the same request. Let me know if you need help to get started with cql-pytest. Or, you can choose one of our other unit-test frameworks (the textual test/cql, or C++ unit tests in test/boost et al.)/

Ok, I'll add a test for this.

[Pavel Solodovnikov]

There's no point in copying the `_specs` vector by value in such
case, just return a const reference. All existing uses create
a copy either way.

Tests: unit(dev)

Signed-off-by: Pavel Solodovnikov <pa.solo...@scylladb.com>
---

cql3/variable_specifications.cc | 4 ++--
cql3/variable_specifications.hh | 2 +-

2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/cql3/variable_specifications.cc b/cql3/variable_specifications.cc
index b8e21d1225..58d9e86a6d 100644
--- a/cql3/variable_specifications.cc
+++ b/cql3/variable_specifications.cc
@@ -57,8 +57,8 @@ size_t variable_specifications::size() const {
return _variable_names.size();
}

-std::vector<lw_shared_ptr<column_specification>> variable_specifications::get_specifications() const & {
- return std::vector<lw_shared_ptr<column_specification>>(_specs.begin(), _specs.end());
+const std::vector<lw_shared_ptr<column_specification>>& variable_specifications::get_specifications() const & {
+ return _specs;
}

std::vector<lw_shared_ptr<column_specification>> variable_specifications::get_specifications() && {

diff --git a/cql3/variable_specifications.hh b/cql3/variable_specifications.hh
index e261851e1d..e8fb3e8aec 100644
--- a/cql3/variable_specifications.hh
+++ b/cql3/variable_specifications.hh