cql: USING TTL 0 means unlimited, not default TTL

Our documentation states that writing an item with "USING TTL 0" means it
should never expire. This should be true even if the table has a default
TTL. But Scylla mistakenly handled "USING TTL 0" exactly like having no
USING TTL at all (i.e., it took the default TTL, instead of unlimited).
We had two xfailing tests demonstrating that Scylla's behavior in this
is different from Cassandra. Scylla's behavior in this case was also
undocumented.

By the way, Cassandra used to have the same bug (CASSANDRA-11207) but
it was fixed already in 2016 (Cassandra 3.6).

So in this patch we fix Scylla's "USING TTL 0" behavior to match the
documentation and Cassandra's behavior since 2016. One xfailing test
starts to pass and the second test passes this bug and fails on a
different one. This patch also adds a third test for "USING TTL ?"
with UNSET_VALUE - it behaves, on both Scylla and Cassandra, like a
missing "USING TTL".

The origin of this bug was that after parsing the statement, we saved
the USING TTL in an integer, and used 0 for the case of no USING TTL
given. This meant that we couldn't tell if we have USING TTL 0 or
no USING TTL at all. This patch uses an std::optional so we can tell
the case of a missing USING TTL from the case of USING TTL 0.

Fixes #6447

Signed-off-by: Nadav Har'El <nyh@scylladb.com>

Closes #13079

cql3/attributes.cc

@@ -10,6 +10,7 @@
 
 #include "cql3/attributes.hh"
 #include "cql3/column_identifier.hh"
+#include <optional>
 
 namespace cql3 {
 
@@ -55,9 +56,9 @@ int64_t attributes::get_timestamp(int64_t now, const query_options& options) {
     }
 }
 
-int32_t attributes::get_time_to_live(const query_options& options) {
+std::optional<int32_t> attributes::get_time_to_live(const query_options& options) {
     if (!_time_to_live.has_value() || _time_to_live_unset_guard.is_unset(options))
-        return 0;
+        return std::nullopt;
 
     cql3::raw_value tval = expr::evaluate(*_time_to_live, options);
     if (tval.is_null()) {

cql3/attributes.hh

@@ -45,7 +45,7 @@ public:
 
     int64_t get_timestamp(int64_t now, const query_options& options);
 
-    int32_t get_time_to_live(const query_options& options);
+    std::optional<int32_t> get_time_to_live(const query_options& options);
 
     db::timeout_clock::duration get_timeout(const query_options& options) const;
 

cql3/statements/modification_statement.cc

@@ -17,6 +17,7 @@
 #include "cql3/util.hh"
 #include "validation.hh"
 #include "db/consistency_level_validations.hh"
+#include <optional>
 #include <seastar/core/shared_ptr.hh>
 #include <boost/range/adaptor/transformed.hpp>
 #include <boost/range/adaptor/map.hpp>
@@ -95,8 +96,9 @@ bool modification_statement::is_timestamp_set() const {
     return attrs->is_timestamp_set();
 }
 
-gc_clock::duration modification_statement::get_time_to_live(const query_options& options) const {
-    return gc_clock::duration(attrs->get_time_to_live(options));
+std::optional<gc_clock::duration> modification_statement::get_time_to_live(const query_options& options) const {
+    std::optional<int32_t> ttl = attrs->get_time_to_live(options);
+    return ttl ? std::make_optional<gc_clock::duration>(*ttl) : std::nullopt;
 }
 
 future<> modification_statement::check_access(query_processor& qp, const service::client_state& state) const {

cql3/statements/modification_statement.hh

@@ -127,7 +127,7 @@ public:
 
     bool is_timestamp_set() const;
 
-    gc_clock::duration get_time_to_live(const query_options& options) const;
+    std::optional<gc_clock::duration> get_time_to_live(const query_options& options) const;
 
     virtual future<> check_access(query_processor& qp, const service::client_state& state) const override;
 

cql3/update_parameters.hh

@@ -86,7 +86,7 @@ public:
     };
     // Note: value (mutation) only required to contain the rows we are interested in
 private:
-    const gc_clock::duration _ttl;
+    const std::optional<gc_clock::duration> _ttl;
     // For operations that require a read-before-write, stores prefetched cell values.
     // For CAS statements, stores values of conditioned columns.
     // Is a reference to an outside prefetch_data container since a CAS BATCH statement
@@ -99,7 +99,7 @@ public:
     const query_options& _options;
 
     update_parameters(const schema_ptr schema_, const query_options& options,
-            api::timestamp_type timestamp, gc_clock::duration ttl, const prefetch_data& prefetched)
+            api::timestamp_type timestamp, std::optional<gc_clock::duration> ttl, const prefetch_data& prefetched)
         : _ttl(ttl)
         , _prefetched(prefetched)
         , _timestamp(timestamp)
@@ -120,11 +120,7 @@ public:
     }
 
     atomic_cell make_cell(const abstract_type& type, const raw_value_view& value, atomic_cell::collection_member cm = atomic_cell::collection_member::no) const {
-        auto ttl = _ttl;
-
-        if (ttl.count() <= 0) {
-            ttl = _schema->default_time_to_live();
-        }
+        auto ttl = this->ttl();
 
         return value.with_value([&] (const FragmentedView auto& v) {
             if (ttl.count() > 0) {
@@ -136,11 +132,7 @@ public:
     };
 
     atomic_cell make_cell(const abstract_type& type, const managed_bytes_view& value, atomic_cell::collection_member cm = atomic_cell::collection_member::no) const {
-        auto ttl = _ttl;
-
-        if (ttl.count() <= 0) {
-            ttl = _schema->default_time_to_live();
-        }
+        auto ttl = this->ttl();
 
         if (ttl.count() > 0) {
             return atomic_cell::make_live(type, _timestamp, value, _local_deletion_time + ttl, ttl, cm);
@@ -162,7 +154,7 @@ public:
     }
 
     gc_clock::duration ttl() const {
-        return _ttl.count() > 0 ? _ttl : _schema->default_time_to_live();
+        return _ttl.value_or(_schema->default_time_to_live());
     }
 
     gc_clock::time_point expiry() const {

test/cql-pytest/cassandra_tests/validation/operations/insert_test.py

@@ -161,7 +161,7 @@ def testInsertWithAStaticColumn(cql, test_keyspace, forceFlush):
                              "INSERT INTO %s (partitionKey, clustering_2, staticValue) VALUES (0, 0, 'A')")
 
 # Reproduces #6447 and #12243:
-@pytest.mark.xfail(reason="Issue #6447, #12243")
+@pytest.mark.xfail(reason="Issue #12243")
 def testInsertWithDefaultTtl(cql, test_keyspace):
     secondsPerMinute = 60
     with create_table(cql, test_keyspace, f"(a int PRIMARY KEY, b int) WITH default_time_to_live = {10*secondsPerMinute}") as table:

test/cql-pytest/test_ttl.py

@@ -7,6 +7,7 @@
 #############################################################################
 
 from util import new_test_table, unique_key_int
+from cassandra.query import UNSET_VALUE
 import pytest
 import time
 
@@ -45,10 +46,25 @@ def test_basic_default_ttl(cql, table_ttl_1):
 # ttl set for the table. Here we check that also in the special case of
 # "using ttl 0" (which means the item should never expire) it also overrides
 # the default TTL. Reproduces issue #9842.
-@pytest.mark.xfail(reason="Issue #9842")
 def test_default_ttl_0_override(cql, table_ttl_100):
     p = unique_key_int()
     cql.execute(f'INSERT INTO {table_ttl_100} (p, v) VALUES ({p}, 1) USING TTL 0')
     # We can immediately check that this item's TTL is "null", meaning it
     # will never expire. There's no need to have any sleeps.
     assert list(cql.execute(f'SELECT ttl(v) from {table_ttl_100} where p={p}')) == [(None,)]
+
+# Whereas a zero TTL overrides the default TTL, an unset TTL doesn't - and
+# leaves the default TTL. This means that the text in Cassandra's NEWS.txt
+# which claims that "an unset bind ttl is treated as 'unlimited'" is not
+# accurate - it's only unlimited if there is no default TTL for the table,
+# and if there is, this default TTL is used instead of unlimited. In other
+# words, an UNSET_VALUE TTL behaves exactly like not specifying a TTL at all.
+# See also test_unset.py::test_unset_ttl()
+def test_default_ttl_unset_override(cql, table_ttl_100):
+    p = unique_key_int()
+    stmt = cql.prepare(f'INSERT INTO {table_ttl_100} (p, v) VALUES ({p}, 1) USING TTL ?')
+    cql.execute(stmt, [UNSET_VALUE])
+    # At this point, reading ttl(v) would normally return 99, but it's
+    # possible in case of delays to be somewhat lower. But it wouldn't
+    # be None - which would mean that the item will never expire.
+    assert list(cql.execute(f'SELECT ttl(v) from {table_ttl_100} where p={p}')) != [(None,)]

test/cql-pytest/test_unset.py

@@ -115,6 +115,10 @@ def test_unset_list_append(cql, table1, cassandra_bug):
 
 # According to Cassandra's NEWS.txt, "an unset bind ttl is treated as
 # 'unlimited'". It shouldn't skip the write.
+# Note that the NEWS.txt is not accurate: An unset ttl isn't really treated
+# as unlimited, but rather as the default ttl set on the table. The default
+# ttl is usually unlimited, but not always. We test that case in
+# test_ttl.py::test_default_ttl_unset()
 def test_unset_ttl(cql, table1):
     p = unique_key_int()
     # First write using a normal TTL: