[tarantool-patches] Re: [PATCH] sql: change of PRAGMA INDEX_INFO syntax

["n.pettik" <korablev@tarantool.org>]

In general LGTM, several minor nits.

> This change removes 'pragma index_xinfo' syntax. 'pragma index_info'
> now works as 'pragma index_xinfo' and also displays type of columns in
> index.
> 
> Cleaned up pragma column names array (pragma.h::pragCName).
> 
> Fixes #3194
> —

Could you rebase on the latest 2.0? At current state I can’t build it on MacOS:

[  0%] Building C object src/lib/csv/CMakeFiles/csv.dir/csv.c.o
Scanning dependencies of target json_path
[  1%] Built target mkkeywordhash
[  1%] Building C object src/lib/salad/CMakeFiles/salad.dir/rope.c.o
[  1%] Building C object src/lib/salad/CMakeFiles/salad.dir/rtree.c.o
[  1%] Building C object src/lib/salad/CMakeFiles/salad.dir/guava.c.o
[  1%] Building C object src/lib/salad/CMakeFiles/salad.dir/bloom.c.o
[  1%] Building C object src/lib/json/CMakeFiles/json_path.dir/path.c.o
[  2%] Built target lemon
[  2%] Building C object CMakeFiles/misc.dir/third_party/base64.c.o
[  2%] Building C object CMakeFiles/misc.dir/third_party/qsort_arg.c.o
[  2%] Building C object CMakeFiles/ev.dir/third_party/tarantool_ev.c.o
[  2%] Built target eio
[  5%] Built target zstd
Scanning dependencies of target uri
Scanning dependencies of target crc32
Scanning dependencies of target small
Scanning dependencies of target bit
[  5%] Building C object src/lib/msgpuck/CMakeFiles/msgpuck.dir/msgpuck.c.o
error: unknown warning option '-Wno-cast-function-type'; did you mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]
[  6%] Building C object src/lib/msgpuck/CMakeFiles/msgpuck.dir/hints.c.o
error: unknown warning option '-Wno-cast-function-type'; did you mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]
make[2]: *** [src/lib/salad/CMakeFiles/salad.dir/rtree.c.o] Error 1
make[2]: *** Waiting for unfinished jobs....
error: unknown warning option '-Wno-cast-function-type'; did you make[2]: *** [src/lib/salad/CMakeFiles/salad.dir/rope.c.o] Error 1
mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]
error: unknown warning option '-Wno-cast-function-type'; did you mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]
make[2]: *** [src/lib/json/CMakeFiles/json_path.dir/path.c.o] Error 1
error: unknown warning option '-Wno-cast-function-type'; did you mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]
make[2]: *** [src/lib/msgpuck/CMakeFiles/msgpuck.dir/msgpuck.c.o] Error 1
make[2]: *** Waiting for unfinished jobs....
error: unknown warning option '-Wno-cast-function-type'; did you mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]
make[2]: *** [src/lib/msgpuck/CMakeFiles/msgpuck.dir/hints.c.o] Error 1
error: unknown warning option '-Wno-cast-function-type'; did you mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]
[  6%] Building CXX object test/unit/CMakeFiles/reflection_cxx.test.dir/reflection_cxx.cc.o
[  6%] Building C object test/unit/CMakeFiles/reflection_cxx.test.dir/unit.c.o
[  6%] Building CXX object test/unit/CMakeFiles/int96.test.dir/int96.cc.o
[  6%] Building C object test/unit/CMakeFiles/reflection_cxx.test.dir/__/__/src/reflection.c.o
[  6%] Building C object test/unit/CMakeFiles/find_path.test.dir/__/__/src/find_path.c.o
[  6%] Building C object test/unit/CMakeFiles/find_path.test.dir/find_path.c.o
errorerror: : unknownunknown  warningwarning  optionoption  '-Wno-cast-function-type';'-Wno-cast-function-type';  diddid  youyou  meanmean  '-Wno-bad-function-cast'?'-Wno-bad-function-cast'?  [-Werror,-Wunknown-warning-option][-Werror,-Wunknown-warning-option]
...
Etc.

> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 473cf89d8..042823267 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -1286,7 +1286,7 @@ cmd ::= PRAGMA nm(X) EQ minus_num(Y).        {
> cmd ::= PRAGMA nm(X) LP minus_num(Y) RP.     {
>     sqlite3Pragma(pParse,&X,&Y,0,1);
> }
> -cmd ::= PRAGMA nm(X) EQ nm(Z) DOT nm(Y).    {
> +cmd ::= PRAGMA nm(X) LP nm(Z) DOT nm(Y) RP.  {
>     sqlite3Pragma(pParse,&X,&Y,&Z,0);
> }
> cmd ::= PRAGMA .                            {
> diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
> index 4f64ab6f2..0eacef5c0 100644
> --- a/src/box/sql/pragma.c
> +++ b/src/box/sql/pragma.c
> @@ -322,8 +322,7 @@ sql_pragma_table_stats(struct space *space, void *data)
> }
> 
> /**
> - * This function handles PRAGMA INDEX_INFO and PRAGMA INDEX_XINFO
> - * statements.
> + * This function handles PRAGMA INDEX_INFO statement.

Nit: it would be great if you put here (explanation of) format of
displayed output. For example see sql_pragma_table_info().

>  *
>  * @param parse Current parsing content.
>  * @param pragma Definition of index_info pragma.
> @@ -349,32 +348,26 @@ sql_pragma_index_info(struct Parse *parse, const PragmaName *pragma,
> 		return;
> 	struct index *idx = space_index(space, iid);
> 	assert(idx != NULL);
> -	/* PRAGMA index_xinfo (more informative version). */
> -	if (pragma->iArg > 0) {
> -		parse->nMem = 6;
> -	} else {
> -		/* PRAGMA index_info ... */
> -		parse->nMem = 3;
> -	}
> +	parse->nMem = 7;
> 	struct Vdbe *v = sqlite3GetVdbe(parse);
> 	assert(v != NULL);
> 	uint32_t part_count = idx->def->key_def->part_count;
> 	assert(parse->nMem <= pragma->nPragCName);
> 	struct key_part *part = idx->def->key_def->parts;
> 	for (uint32_t i = 0; i < part_count; i++, part++) {
> -		sqlite3VdbeMultiLoad(v, 1, "iis", i, part->fieldno,
> -				     space->def->fields[part->fieldno].name);
> -		if (pragma->iArg > 0) {
> -			const char *c_n;
> -			uint32_t id = part->coll_id;
> -			struct coll *coll = part->coll;
> -			if (coll != NULL)
> -				c_n = coll_by_id(id)->name;
> -			else
> -				c_n = "BINARY";
> -			sqlite3VdbeMultiLoad(v, 4, "isi", part->sort_order,
> -					     c_n, i < part_count);
> -		}
> +		const char *c_n;
> +		uint32_t id = part->coll_id;
> +		struct coll *coll = part->coll;
> +		if (coll != NULL)
> +			c_n = coll_by_id(id)->name;
> +		else
> +			c_n = "BINARY";
> +		uint32_t fieldno = part->fieldno;
> +		enum field_type type = space->def->fields[fieldno].type;
> +		sqlite3VdbeMultiLoad(v, 1, "iisisis", i, fieldno,
> +				     space->def->fields[fieldno].name,
> +				     part->sort_order, c_n, i < part_count,

But i < part_count is always true, isn’t it?

> /* Definitions of all built-in pragmas */
> @@ -97,11 +82,13 @@ typedef struct PragmaName {
> 	u8 nPragCName;		/* Num of col names. 0 means use pragma name */
> 	u32 iArg;		/* Extra argument */
> } PragmaName;
> +/* The order of pragmas in this array is important: it has */
> +/* to be sorted. For more info see pragma_locate function. */

Nit:

-/* The order of pragmas in this array is important: it has */
-/* to be sorted. For more info see pragma_locate function. */
+
+/**
+ * The order of pragmas in this array is important: it has
+ * to be sorted. For more info see pragma_locate function.
+ */