Re: [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 4 comments below.

On 11/12/2019 14:44, Nikita Pettik wrote:
> Each column of result set can feature its name alias. For instance:
> 
> SELECT x + 1 AS add FROM ...;
> 
> In this case real name of resulting set column is "x + 1" meanwhile
> "add" is its alias. This patch extends metadata with optional metadata
> member which corresponds to column's alias.
> 
> Closes #4407
> 
> @TarantoolBot document
> Title: extended SQL metadata
> 
> Before this patch metadata for SQL DQL contained only two fields:
> name and type of each column of result set. Now it may contain
> following properties:
>  - collation (in case type of resulting set column is string and
>               collation is different from default "none");
>    is encoded with IPROTO_FIELD_COLL (0x2) key in IPROTO_METADATA map;
>    in msgpack is encoded as string and held with MP_STR type;
>  - is_nullable (in case column of result set corresponds to space's
>                 field; for expressions like x+1 for the sake of
>                 simplicity nullability is omitted);
>    is encoded with IPROTO_FIELD_IS_NULLABLE key (0x3) in IPROTO_METADATA;
>    in msgpack is encoded as boolean and held with MP_BOOL type;
>    note that absence of this field implies that nullability is unknown;
>  - is_autoincrement (is set only for autoincrement column in result
>                      set);
>    is encoded with IPROTO_FIELD_IS_AUNTOINCREMENT (0x4) key in IPROTO_METADATA;
>    in msgpack is encoded as boolean and held with MP_BOOL type;
>  - alias (if column of result set is specified with AS label);
>    is encoded with IPROTO_FIELD_ALIAS (0x5) key in IPROTO_METADATA map;
>    in msgpack is encoded as string and held with MP_STR type;
>    note that if there's no
> 
> This extended metadata is send only when PRAGMA full_metadata is
> enabled. Otherwise, only basic (name and type) metadata is processed.

1. You didn't mention, that the PRAGMA not only regulates what
metadata to return, but also changes 'name' field behaviour:

box.execute('PRAGMA full_metadata = true;')
tarantool> box.execute('SELECT 1 AS label')
---
- metadata:
  - type: integer
    name: '1'
    alias: LABEL
  rows:
  - [1]
...

box.execute('PRAGMA full_metadata = false;')
tarantool> box.execute('SELECT 1 AS label')
---
- metadata:
  - name: LABEL
    type: integer
  rows:
  - [1]
...

I don't think it is ok though. Not the missing doc, but the
not stable 'name'. It is one another reason to keep the 'name'
as is and return the original expression span in a new column.
'Orig_name', 'oname', 'span', 'column', 'expr', etc.

> ---
>  src/box/execute.c               | 18 ++++++++++++++----
>  src/box/iproto_constants.h      |  1 +
>  src/box/lua/execute.c           |  9 +++++++--
>  src/box/lua/net_box.c           |  6 +++++-
>  src/box/sql/select.c            | 32 ++++++++++++++++++++++++++++----
>  src/box/sql/sqlInt.h            |  3 +++
>  src/box/sql/vdbe.h              |  3 +++
>  src/box/sql/vdbeInt.h           |  1 +
>  src/box/sql/vdbeapi.c           |  8 ++++++++
>  src/box/sql/vdbeaux.c           | 15 +++++++++++++++
>  test/sql/full_metadata.result   | 38 +++++++++++++++++++++++++++++++++++---
>  test/sql/full_metadata.test.lua |  7 +++++++
>  12 files changed, 127 insertions(+), 14 deletions(-)
> > diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index d92da4d8e..c1770e7b4 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1827,7 +1827,19 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
>  			zCol = space_def->fields[iCol].name;
>  			const char *name = NULL;
>  			if (pEList->a[i].zName != NULL) {
> -				name = pEList->a[i].zName;
> +				if (is_full_meta) {
> +					const char *alias = NULL;
> +					if (pEList->a[i].zSpan != NULL) {
> +						alias = pEList->a[i].zName;
> +						name = pEList->a[i].zSpan;
> +					} else {
> +						alias = pEList->a[i].zName;
> +						name = pEList->a[i].zName;

2. What is a point of assigning alias to the same value
in both branches of 'if'? Couldn't you do it out of
the 'if' and drop {} ? The same below.

> +					}
> +					vdbe_metadata_set_col_alias(v, i, alias);
> +				} else {
> +					name = pEList->a[i].zName;
> +				}
>  			} else {
>  				if (!shortNames && !fullNames) {
>  					name = pEList->a[i].zSpan;
> @@ -1854,9 +1866,21 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
>  			}
>  		} else {
>  			const char *z = NULL;
> -			if (pEList->a[i].zName != NULL)
> -				z = pEList->a[i].zName;
> -			else if (pEList->a[i].zSpan != NULL)
> +			if (pEList->a[i].zName != NULL) {
> +				if (is_full_meta ) {

3. Extra whitespace after is_full_meta.

> +					const char *alias = NULL;
> +					if (pEList->a[i].zSpan != NULL) {
> +						alias = pEList->a[i].zName;
> +						z = pEList->a[i].zSpan;
> +					} else {
> +						alias = pEList->a[i].zName;
> +						z = pEList->a[i].zName;
> +					}
> +					vdbe_metadata_set_col_alias(v, i, alias);
> +				} else {
> +					z = pEList->a[i].zName;
> +				}
> +			} else if (pEList->a[i].zSpan != NULL)
>  				z = pEList->a[i].zSpan;
>  			else
>  				z = tt_sprintf("column%d", i + 1);
> diff --git a/test/sql/full_metadata.result b/test/sql/full_metadata.result
> index 7c2982682..8f0bced36 100644
> --- a/test/sql/full_metadata.result
> +++ b/test/sql/full_metadata.result
> @@ -118,6 +121,35 @@ execute("SELECT * FROM t;")
>   |   - [1, 1, 'aSd']
>   | ...
>  
> +-- Alias is always set in extended metadata. If column label in
> +-- form of AS clause is set, then this alias is presented in
> +-- metadata. Otherwise, alias is just the same as name.

4. But it is not true. The alias is not set always:

=========================================================

tarantool> box.execute('PRAGMA full_metadata = true;')
---
- row_count: 0
...

tarantool> box.execute('SELECT 1')
---
- metadata:
  - name: '1'
    type: integer
  rows:
  - [1]
...

=========================================================

As you can see, no AS = no alias. This is what I don't like
in this patch the most. A user does not have a reliable always
present column with a displayable name: either the column
span or its alias after AS. He can't use name, because it
changes its behavour depending on AS. And he can't use alias,
because sometimes it is not present. That leads to confusion,
and necessity to branch in user code a lot.

The same example shows, that alias is a bad name. Here you need
to return it, but it is not alias like AS. 'Label' would fit
better here.