alternator: initial implementation of "+" and "-" in UpdateExpression

This patch implements the last (finally!) syntactic feature of the
UpdateExpression - the ability to do SET a=val1+val2 (where, as
before, each of the values can be a reference to a value, an
attribute path, or a function call).

The implementation is not perfect: It adds the values as double-precision
numbers, which can lose precision. So the patch adds a new test which
checks that the precision isn't lost - a test that currently fails
(xfail) on Alternator, but passes on DynamoDB. The pre-existing test
for adding small integer now passes on Alternator.

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

alternator-test/test_update_expression.py

@@ -5,6 +5,7 @@ import string
 
 import pytest
 from botocore.exceptions import ClientError
+from decimal import Decimal
 
 def random_string(length=10, chars=string.ascii_uppercase + string.digits):
     return ''.join(random.choice(chars) for x in range(length))
@@ -319,7 +320,6 @@ def test_update_expression_non_existant_clause(test_table_s):
 # Test support for "SET a = :val1 + :val2", "SET a = :val1 - :val2"
 # Only exactly these combinations work - e.g., it's a syntax error to
 # try to add three. Trying to add a string fails.
-@pytest.mark.xfail(reason="plus and minus in SET not yet implemented")
 def test_update_expression_plus_basic(test_table_s):
     p = random_string()
     test_table_s.update_item(Key={'p': p},
@@ -335,15 +335,33 @@ def test_update_expression_plus_basic(test_table_s):
         test_table_s.update_item(Key={'p': p},
             UpdateExpression='SET b = :val1 + :val2 + :val3',
             ExpressionAttributeValues={':val1': 4, ':val2': 3, ':val3': 2})
-    # Trying to add a string value fails, saying "Incorrect operand type for
+    # Only numeric values can be added - other things like strings or lists
+    # cannot be added, and we get an error like "Incorrect operand type for
     # operator or function; operator or function: +, operand type: S".
-    # Interestingly, this implies DynamoDB just handles + and - similarly to a
-    # two-parameter function, and we can do this in our implementation as well.
-    # But not quite: see test_update_expression_function_plus_nesting.
     with pytest.raises(ClientError, match='ValidationException'):
         test_table_s.update_item(Key={'p': p},
             UpdateExpression='SET b = :val1 + :val2',
             ExpressionAttributeValues={':val1': 'dog', ':val2': 3})
+    with pytest.raises(ClientError, match='ValidationException'):
+        test_table_s.update_item(Key={'p': p},
+            UpdateExpression='SET b = :val1 + :val2',
+            ExpressionAttributeValues={':val1': ['a', 'b'], ':val2': ['1', '2']})
+
+# While most of the Alternator code just saves high-precision numbers
+# unchanged, the "+" and "-" operations need to calculate with them, and
+# we should check the calculation isn't done with some lower-precision
+# representation, e.g., double
+@pytest.mark.xfail(reason="plus and minus implemented with wrong precision")
+def test_update_expression_plus_precision(test_table_s):
+    p = random_string()
+    test_table_s.update_item(Key={'p': p},
+        UpdateExpression='SET b = :val1 + :val2',
+        ExpressionAttributeValues={':val1': Decimal("1"), ':val2': Decimal("10000000000000000000000")})
+    assert test_table_s.get_item(Key={'p': p}, ConsistentRead=True)['Item'] == {'p': p, 'b': Decimal("10000000000000000000001")}
+    test_table_s.update_item(Key={'p': p},
+        UpdateExpression='SET b = :val2 - :val1',
+        ExpressionAttributeValues={':val1': Decimal("1"), ':val2': Decimal("10000000000000000000000")})
+    assert test_table_s.get_item(Key={'p': p}, ConsistentRead=True)['Item'] == {'p': p, 'b': Decimal("9999999999999999999999")}
 
 # Test support for "SET a = b + :val2" et al., i.e., a version of the
 # above test_update_expression_plus_basic with read before write.

alternator/executor.cc

@@ -414,6 +414,44 @@ static Json::Value list_concatenate(const Json::Value& v1, const Json::Value& v2
     return ret;
 }
 
+// Check if a given JSON object encodes a number (i.e., it is a {"N": [...]}
+// and returns an object representing it.
+// FIXME: returning double is wrong! It loses precision.
+static double unwrap_number(const Json::Value& v) {
+    if (!v.isObject() || v.size() != 1) {
+        throw api_error("ValidationException", "UpdateExpression: invalid number object");
+    }
+    auto it = v.begin();
+    if (it.key() != "N") {
+        throw api_error("ValidationException",
+                format("UpdateExpression: expected number, found type '{}'", it.key()));
+    }
+    if (!it->isString()) {
+        throw api_error("ValidationException", "UpdateExpression: improperly formatted number constant");
+    }
+    // FIXME: to not lose precision, we really need to do something like:
+    // return decimal_type->from_string(it->asString());
+    return std::stod(it->asString());
+}
+
+// Take two JSON-encoded numeric values ({"N": "thenumber"}) and return the
+// sum, again as a JSON-encoded number.
+static Json::Value number_add(const Json::Value& v1, const Json::Value& v2) {
+    auto n1 = unwrap_number(v1);
+    auto n2 = unwrap_number(v2);
+    Json::Value ret(Json::objectValue);
+    ret["N"] = n1 + n2; // FIXME: should convert to string without losing precision.
+    return ret;
+}
+
+static Json::Value number_subtract(const Json::Value& v1, const Json::Value& v2) {
+    auto n1 = unwrap_number(v1);
+    auto n2 = unwrap_number(v2);
+    Json::Value ret(Json::objectValue);
+    ret["N"] = n1 - n2; // FIXME: should convert to string without losing precision.
+    return ret;
+}
+
 // Given a parsed::value, which can refer either to a constant value from
 // ExpressionAttributeValues, to the value of some attribute, or to a function
 // of other values, this function calculates the resulting value.
@@ -455,6 +493,30 @@ static Json::Value calculate_value(const parsed::value& v,
 
 }
 
+// Same as calculate_value() above, except takes a set_rhs, which may be
+// either a single value, or v1+v2 or v1-v2.
+static Json::Value calculate_value(const parsed::set_rhs& rhs,
+        const Json::Value& expression_attribute_values,
+        std::unordered_set<std::string>& used_attribute_values) {
+    switch(rhs._op) {
+    case 'v':
+        return calculate_value(rhs._v1, expression_attribute_values, used_attribute_values);
+    case '+': {
+        Json::Value v1 = calculate_value(rhs._v1, expression_attribute_values, used_attribute_values);
+        Json::Value v2 = calculate_value(rhs._v2, expression_attribute_values, used_attribute_values);
+        return number_add(v1, v2);
+    }
+    case '-': {
+        Json::Value v1 = calculate_value(rhs._v1, expression_attribute_values, used_attribute_values);
+        Json::Value v2 = calculate_value(rhs._v2, expression_attribute_values, used_attribute_values);
+        return number_subtract(v1, v2);
+    }
+    }
+    // Can't happen
+    return Json::Value::null;
+}
+
+
 future<json::json_return_type> executor::update_item(std::string content) {
     _stats.api_operations.update_item++;
     Json::Value update_info = json::to_json_value(content);

alternator/expressions.g

@@ -157,13 +157,15 @@ update_expression_set_value returns [parsed::value v]:
      ')'
     ;
 
-// TODO: rhs can also be value+value or value-value.
-update_expression_set_rhs returns [parsed::value v]:
-    update_expression_set_value  { $v = $update_expression_set_value.v; }
+update_expression_set_rhs returns [parsed::set_rhs rhs]:
+    v=update_expression_set_value  { $rhs.set_value(std::move($v.v)); }
+    (   '+' v=update_expression_set_value  { $rhs.set_plus(std::move($v.v)); }
+      | '-' v=update_expression_set_value  { $rhs.set_minus(std::move($v.v)); }
+    )?
     ;
 
 update_expression_set_action returns [parsed::update_expression::action a]:
-    path '=' rhs=update_expression_set_rhs { $a.assign_set($path.p, $rhs.v); };
+    path '=' rhs=update_expression_set_rhs { $a.assign_set($path.p, $rhs.rhs); };
 
 update_expression_remove_action returns [parsed::update_expression::action a]:
     path { $a.assign_remove($path.p); };

alternator/expressions_types.hh

@@ -57,7 +57,8 @@ public:
 // the request (":val"), a path to an attribute from the existing item
 // (e.g., "a.b[3].c"), or a function of other such values.
 // Note that the real right-hand-side of an assignment is actually a bit
-// more general - it allows either a value, or a value+value or value-value.
+// more general - it allows either a value, or a value+value or value-value -
+// see class set_rhs below.
 class value {
 public:
     struct function_call {
@@ -99,13 +100,33 @@ public:
     }
 };
 
+// The right-hand-side of a SET in an update expression can be either a
+// single value (see above), or value+value, or value-value.
+class set_rhs {
+public:
+    char _op;  // '+', '-', or 'v''
+    value _v1;
+    value _v2;
+    void set_value(value&& v1) {
+        _op = 'v';
+        _v1 = std::move(v1);
+    }
+    void set_plus(value&& v2) {
+        _op = '+';
+        _v2 = std::move(v2);
+    }
+    void set_minus(value&& v2) {
+        _op = '-';
+        _v2 = std::move(v2);
+    }
+};
+
 class update_expression {
 public:
     struct action {
         path _path;
         struct set {
-            // TODO: needs to be rhs type, not just value (also value+value, value-value)
-            value _rhs;
+            set_rhs _rhs;
         };
         struct remove {
         };
@@ -118,9 +139,9 @@ public:
         std::variant<set, remove, add, del> _action;
 
         // FIXME: rhs type, not just one value, also value+value, value-value
-        void assign_set(path p, value v) {
+        void assign_set(path p, set_rhs rhs) {
             _path = std::move(p);
-            _action = set { std::move(v) };
+            _action = set { std::move(rhs) };
         }
         void assign_remove(path p) {
             _path = std::move(p);