[tarantool-patches] Re: [PATCH v3 3/3] Lua: implement json path access to tuple fields

[Vladislav Shpilevoy]

Hello. Thanks for review.

New commit message:

     lua: implement json path access to tuple fields

     Until this moment in Lua a tuple data could be accessed in 3 ways:
     - get field by field number, decode into Lua,
     - get field by field name, decode into Lua
     - decode entire tuple into Lua.

     It was impossible to decode into Lua only a part of the field to
     avoid Lua garbage creating. For example, consider the tuple:
     `{1, 2, {key1 = value1,
              key2 = {key21 = {key211 = {key2111, key2112}}}}}`
     with field names `field1`, `field2`, `field3`.

     To get `key2112` it is necessary to decode into Lua the entire
     3-th field, including `key1`, `value1`, `key2`, `key21`,
     `key211`, `key2111` using the syntax
     `key2112 = tuple.field3.key2.key21.key211[2]`.

     Now the one can use the following syntax:
     `key2112 = tuple["field3.key2.key21.key211[2]"]`. The difference
     is in placing all the path into quotes and brackets `["..."]`.
     The Tarantool goes through this path and MessagePack tuple body,
     gets the needed tuple part and decodes into Lua only it.

     The path must be valid JSON path with one exception - in
     Tarantool a path can start with `.`. For example:
     `tuple[".field3..."]` - it is done to lay emphasis that the JSON
     path here is just a suffix for the tuple.

     At the same time tuple field names still work: `tuple["field1"]`,
     `tuple.field2`.

     If the field name looks like JSON path, for example:
     `my.field.name`, then it works too. The path at first is checked
     to be a real JSON path, and if nothing is found, then the entire
     path is considered as a field name. To combine such names and
     path elements the one can use `["..."]`. For example:
     `tuple["['my.field.name'].key1.key2.['another.key.in.map']"]`.

     Closes #1285

> 
> Why did you move handling of integer index from Lua to C? AFAIU having
> it in Lua should be faster. Besides, this new function is kinda
> difficult to follow IMO as it does two things now. I'd prefer it to do
> just one thing - looking up a tuple field by path.
> 

Fixed. I returned the tuple field by number implementation as it
was before JSON path patch.


diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index c10e9b253..e01c12a56 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -426,8 +426,8 @@ lbox_tuple_transform(struct lua_State *L)
   * @param L Lua state.
   * @param tuple 1-th argument on a lua stack, tuple to get field
   *        from.
- * @param path 2-th argument on lua stack. Can be field name,
- *        JSON path to a field or a field number.
+ * @param path 2-th argument on lua stack. Can be field name or a
+ *        JSON path to a field.
   *
   * @retval not nil Found field value.
   * @retval     nil A field is NULL or does not exist.
@@ -438,37 +438,16 @@ lbox_tuple_field_by_path(struct lua_State *L)
  	struct tuple *tuple = luaT_istuple(L, 1);
  	/* Is checked in Lua wrapper. */
  	assert(tuple != NULL);
-	const char *field = NULL;
-	if (lua_isnumber(L, 2)) {
-		double dbl_index = lua_tonumber(L, 2);
-		if (dbl_index != floor(dbl_index))
-			goto usage_error;
-		int index = (int) floor(dbl_index) - TUPLE_INDEX_BASE;
-		if (index >= 0) {
-			field = tuple_field(tuple, index);
-			if (field == NULL) {
-				lua_pushnil(L);
-				return 1;
-			}
-		} else {
-			lua_pushnil(L);
-			return 1;
-		}
-	} else if (lua_isstring(L, 2)) {
-		size_t len;
-		const char *path = lua_tolstring(L, 2, &len);
-		if (len == 0)
-			goto usage_error;
-		if (tuple_field_by_path(tuple, path, (uint32_t) len,
-					lua_hashstring(L, 2), &field) != 0) {
-			return luaT_error(L);
-		} else if (field == NULL) {
-			lua_pushnil(L);
-			return 1;
-		}
-	} else {
-usage_error:
-		return luaL_error(L, "Usage: tuple[<path> or number >= 1]");
+	assert(lua_isstring(L, 2));
+	size_t len;
+	const char *field = NULL, *path = lua_tolstring(L, 2, &len);
+	if (len == 0)
+		return 0;
+	if (tuple_field_by_path(tuple, path, (uint32_t) len,
+				lua_hashstring(L, 2), &field) != 0) {
+		return luaT_error(L);
+	} else if (field == NULL) {
+		return 0;
  	}
  	assert(field != NULL);
  	luamp_decode(L, luaL_msgpack_default, &field);
diff --git a/src/box/lua/tuple.lua b/src/box/lua/tuple.lua
index 0f368ba9c..63ea73e43 100644
--- a/src/box/lua/tuple.lua
+++ b/src/box/lua/tuple.lua
@@ -296,20 +296,31 @@ end

  methods["__serialize"] = tuple_totable -- encode hook for 
msgpack/yaml/json

+local tuple_field = function(tuple, field_n)
+    local field = builtin.box_tuple_field(tuple, field_n - 1)
+    if field == nil then
+        return nil
+    end
+    -- Use () to shrink stack to the first return value
+    return (msgpackffi.decode_unchecked(field))
+end
+
  ffi.metatype(tuple_t, {
      __len = function(tuple)
          return builtin.box_tuple_field_count(tuple)
      end;
      __tostring = internal.tuple.tostring;
      __index = function(tuple, key)
-        if type(key) == "string" or type(key) == "number" then
-            -- Try to get a field by json path or by [index]. If
-            -- it was not found (rc ~= 0) then return a method
-            -- from the vtable. If a collision occurred, then
-            -- fields have higher priority. For example, if a
-            -- tuple T has a field with name 'bsize', then T.bsize
-            -- returns field value, not tuple_bsize function. To
-            -- access hidden methods use
+        if type(key) == "number" then
+            return tuple_field(tuple, key)
+        elseif type(key) == "string" then
+            -- Try to get a field by JSON path. If it was not
+            -- found (rc ~= 0) then return a method from the
+            -- vtable. If a collision occurred, then fields have
+            -- higher priority. For example, if a tuple T has a
+            -- field with name 'bsize', then T.bsize returns
+            -- field value, not tuple_bsize function. To access
+            -- hidden methods use
              -- 'box.tuple.<method_name>(T, [args...])'.
              local res = tuple_field_by_path(tuple, key)
              if res ~= nil then