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

[Nikita Pettik <korablev@tarantool.org>]

On 18 Dec 01:29, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 4 comments below.
> 
> 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.

But behaviour you suggesting contradicts all other DBs.
See my answer at the end of letter.
 
> > > 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.

It makes no sense indeed, refactored:

diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 0baabc62e..f0cf775c4 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1829,14 +1829,11 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
                        const char *name = NULL;
                        if (pEList->a[i].zName != NULL) {
                                if (is_full_meta) {
-                                       const char *alias = NULL;
-                                       if (pEList->a[i].zSpan != NULL) {
-                                               alias = pEList->a[i].zName;
+                                       if (pEList->a[i].zSpan != NULL)
                                                name = pEList->a[i].zSpan;
-                                       } else {
-                                               alias = pEList->a[i].zName;
+                                        else
                                                name = pEList->a[i].zName;
-                                       }
+                                       const char *alias = pEList->a[i].zName;
                                        vdbe_metadata_set_col_alias(v, i, alias);
                                } else {
                                        name = pEList->a[i].zName;
@@ -1866,14 +1863,11 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
                        const char *z = NULL;
                        if (pEList->a[i].zName != NULL) {
                                if (is_full_meta ) {
-                                       const char *alias = NULL;
-                                       if (pEList->a[i].zSpan != NULL) {
-                                               alias = pEList->a[i].zName;
+                                       if (pEList->a[i].zSpan != NULL)
                                                z = pEList->a[i].zSpan;
-                                       } else {
-                                               alias = pEList->a[i].zName;
+                                       else
                                                z = pEList->a[i].zName;
-                                       }
+                                       const char *alias = pEList->a[i].zName;
                                        vdbe_metadata_set_col_alias(v, i, alias);

 
> > @@ -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.

Removed.

> > 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.

Always displaying alias (or label - call it whatever you want)
even in full_metadata mode doesn't seem to be reasonable for me.
Users can easily handle it in their code. Sending one map field
with string value duplicating the name and decoding it may affect
performance much more significant than one NULL check (IMHO).

> 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.

Yep, if user wants (for some reason; I don't have statistics how
often alias value can be useful or/and is used in production code)
to get alias value, one should make a small effort and add one
'if' condition:

if (is_full_metadata && alias == NULL)
    alias = name;

That's it.

Alternatively (I don't take into consideration option of swapping
name and alias as you suggest) we can change current default behaviour
and in 'casual metadata mode' return name as name (not alias):

SELECT 1 AS x; -- would return '1' as name (now it returns 'x' as name). 

But I guess some users may turn out to be confused (in case they
haven't red about full_metadata pragma in docs).

> 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.

I don't care much how to call it. If you prefer 'label', I will rename it.
TBO I also don't care much which approach we should follow: always return
alias/label, fix current name/alias resolution, or swap name and alias
at all. If you insist on certain implementation, let's do it. If you
don't care much as well, let's ask smb else to take a final decision.