Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thank you for the review! My answers and diff below.

On Thu, Dec 09, 2021 at 01:31:39AM +0100, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> >>>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> >>>>> index 74a98c550..789d8906c 100644
> >>>>> --- a/src/box/sql/expr.c
> >>>>> +++ b/src/box/sql/expr.c
> >>>>> @@ -3432,6 +3432,35 @@ expr_code_array(struct Parse *parser, struct Expr *expr, int reg)
> >>>>>  	sqlVdbeAddOp3(vdbe, OP_Array, count, reg, values_reg);
> >>>>>  }
> >>>>>  
> >>>>> +static void
> >>>>> +expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
> >>>>
> >>>> 1. I thought the policy was that we name functions, generating VDBE code,
> >>>> using 'emit' suffix. For instance, `vdbe_emit_map()` or `sql_emit_map()`.
> >>>> Don't know about prefix though. I see both vdbe_ and sql_ are used.
> >>>>
> >>> This is usually true, but this function is actually part of sqlExprCodeTarget().
> >>> I believe these functions were created to make sqlExprCodeTarget() more
> >>> readable. All such functions are named sqlExprCode*(), code*() or
> >>> expr_code _*(), for example: sqlExprCodeGetColumn(), codeReal(),
> >>> expr_code_int().
> >>>
> >>> Since all these functions are static, I think we should drop "expr_" prefix for
> >>> them. Not in this patch, though.
> >>
> >> If functions take Expr as an argument like these do, they could be
> >> considered methods of Expr. In that case dropping the expr_ prefix would
> >> violate our naming convention. It is not about static or global here.
> >>
> >> As an alternative they could be considered as methods of Parse, but
> >> then they would need to have parse_ prefix.
> >>
> >> For 'code' vs 'emit' - 'code' is fine by me as long as it is static. But
> >> if it goes public, then either 'code' or 'emit' must be chosen as one
> >> correct suffix. Not a mix.
> >>
> > After some thought, I think you are right. However, I would suggest removing the
> > parser and vdbe from these functions and converting them to proper struct expr
> > methods. This way we can make these functions return a value (most likely as an
> > "out" argument). For example expr_code_dec() should give us DECIMAL. In this
> > case we can make some improvements, for example we can remove "is_neg" from
> > expr_code_int() and turn it into expr_code_uint(), since we know that this '-'
> > sign will be specified as another expr. Also, since these will be valid expr
> > methods, we can drop "static" from their definition. We then should name them
> > accordingly, for  example "expr_code_dec" may be named "expr_to_dec".
> 
> AFAIU, their value is not only in converting the value, but also in doing
> the sqlVdbeAddOp action. You will need to duplicate this call in all places
> where expr_to_dec() would be used. The aspiration for refactoring this code
> is righteous anyway though.
> 
> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index 55e494332..86de3f98a 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -1438,6 +1438,26 @@ case OP_Array: {
> >  	break;
> >  }
> >  
> > +/**
> > + * Opcode: Map P1 P2 P3 * *
> > + * Synopsis: r[P2] = map(P3@P1)
> > + *
> > + * Construct an MAP value from P1 registers starting at reg(P3).
> > + */
> > +case OP_Map: {
> > +	pOut = &aMem[pOp->p2];
> > +
> > +	uint32_t size;
> > +	struct region *region = &fiber()->gc;
> > +	size_t svp = region_used(region);
> > +	char *val = mem_encode_map(&aMem[pOp->p3], pOp->p1, &size, region);
> > +	if (val == NULL || mem_copy_map(pOut, val, size) != 0) {
> > +		region_truncate(region, svp);
> > +		goto abort_due_to_error;
> > +	}
> 
> You should probably truncate the region regardless of the result. Otherwise
> in case of success you will leak the region inside of the query bit by bit
> while SELECT works:
> 
> 	box.execute('CREATE TABLE test (id INTEGER PRIMARY KEY)')
> 	box.execute('INSERT INTO test VALUES (1), (2), (3)')
> 	box.execute('SELECT {id: id} FROM test')
> 
> Here you will do OP_Map 3 times, all will leave some region memory leaked
> every time. It is freed in the end of execution probably, but it might do
> some big usage while the request is in progress when the row count is much
> bigger than 3.
> 
Thank you! Fixed.

> Btw, worth adding a multirow test. All current map tests select a single row.
There are several tests where the number of selected rows is more than one, for
example map-3 or map-4, but the number of rows is still quite low. I added
another test that selects 1000 rows with maps.



Diff:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 6824f11bf..33207db6b 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1456,6 +1456,7 @@ case OP_Map: {
 		region_truncate(region, svp);
 		goto abort_due_to_error;
 	}
+	region_truncate(region, svp);
 	break;
 }
 
diff --git a/test/sql-tap/map.test.lua b/test/sql-tap/map.test.lua
index 7791ca779..c51880cfd 100755
--- a/test/sql-tap/map.test.lua
+++ b/test/sql-tap/map.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(131)
+test:plan(132)
 
 box.schema.func.create('M1', {
     language = 'Lua',
@@ -1165,6 +1165,18 @@ test:do_test(
         "[{5: 1}]"
     })
 
+-- Make sure that multiple row with maps can be selected properly.
+local maps = {{a1 = 1}}
+local strs = "({'a1': 1})"
+for i = 2, 1000 do maps[i] = {['a'..tostring(i)] = i} end
+for i = 2, 1000 do strs = strs .. string.format(", ({'a%d': %d})", i, i) end
+test:do_execsql_test(
+    "map-15",
+    [[
+        SELECT * FROM (VALUES]]..strs..[[);
+    ]], maps)
+
+
 box.execute([[DROP TABLE t1;]])
 box.execute([[DROP TABLE t;]])