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

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

Thanks for the discussion!

On 06/12/2019 13:50, Nikita Pettik wrote:
> On 06 Dec 01:02, Vladislav Shpilevoy wrote:
>> Thanks for the fixes!
>>
>> On 05/12/2019 12:51, Nikita Pettik wrote:
>>> On 28 Nov 23:41, Vladislav Shpilevoy wrote:
>>>> Thanks for the patch!
>>>>
>>>> On 27/11/2019 13:15, 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.
>>>>
>>>> I was always thinking that the alias should be returned as a
>>>> name. And the real name should be returned as meta. And looks
>>>> like it is so:
>>>>
>>>>     tarantool> box.execute('SELECT 1 AS kek')
>>>>     ---
>>>>     - metadata:
>>>>       - name: KEK
>>>>         type: integer
>>>>       rows:
>>>>       - [1]
>>>>     ...
>>>>
>>>> That makes me think we should not break it. And
>>>> meta should return the real name in case there is
>>>> an alias. Because otherwise the aliases are useless
>>>> in meta.
>>>
>>> (ANSI parts which concern Java and CLI are quite complicated to read
>>>  and understand, so I refer to Oracle docs).
>>>
>>> https://docs.oracle.com/javase/8/docs/api/java/sql/ResultSetMetaData.html#getColumnLabel-int-
>>>
>>> 'The suggested title is usually specified by the SQL AS clause.'
>>>
>>> https://stackoverflow.com/questions/4271152/getcolumnlabel-vs-getcolumnname
>>>
>>> I assume that :getColumnLabel() returns name of label, not real name
>>> (at least it seems to be rational).
>>>  
>>
>> I discussed it with Alexander. And that subject is complicated.
>> I don't know exactly what do we need to return. All the drivers
>> work differently.
>>
>> Here is what I understood from the discussion:
>>
>>     Metadata contains 'label' and 'name'.
>>     There are 2 cases: SELECTed column is an expression, or a
>>     table column.
>>
>>     - In case the result set column is an expression, the
>>       label is the value after 'AS'. If the alias is not
>>       specified, the label may be anything. For example,
>>       the expression string representation. 'Name' is
>>       undefined.
> 
> Why name is undefined? I'd say vice versa - alias is undefined
> (for the simplicity sake let's say it equals to name); name is
> string representation (in most cases). 
> 
> https://grokbase.com/t/postgresql/pgsql-jdbc/047wwctbyf/wrong-column-names-in-resultsetmetadata
> '''
> Most return the same thing as getColumnName(); not surprising
> since that's a good default display title.
> '''

Firstly, there is no 'alias' anywhere. Nobody returns a meta column,
which is just alias or nothing.

Talking of name and label - well, that is what I said. Label is
always defined, and it is a title to display. Name may be
undefined in case it is not a table column, but a constant or an
expression.

In Tarantool it is vice versa. Our IPROTO_FIELD_NAME is always
defined and is in fact a label. And we call it 'name' in fronend.
We just need to add table column name now, and call it somehow
different. So as not to break compatibility. Your patch breaks it.

> 
> Some time ago, there was discussion in dev mailing list concerning
> default column names btw.
> 

I don't remember, or likely I didn't participate. I started
digging into that only now. If you have some points, you need
to bring them up. Alexander has found some discussions in Telegram,
and we talked through them privately. This is from where my
thoughts came, and the links you provided just prove them.

>>     - In case the result set column is a table column,
>>       label is the value after 'AS'. If the alias is not
>>       specified, it is the original column name (i.e. the
>>       value before 'AS'). 'Name' is the original column
>>       name.
>>
>> 'Label' is just something printable to show to a user.
>> 'Name' is something functional. This may be used to generate
>> an update request.
> 
> For me it sounds extremely wierd, I can't realize how did
> you come up with these thoughts...Could you please provide
> examples of DBs/drivers where such behavior can be observer?

MariaDB, MySQL. At least these examples I've seen.

> The only thing I found is an ancient discussion in PostgreSQL
> mailing list:
> https://grokbase.com/t/postgresql/pgsql-jdbc/047wwctbyf/wrong-column-names-in-resultsetmetadata
> 
> All other resources I've visited say that alias is considered to be the
> indentifier coming after AS clause; name is always indentifier that
> is specified in result set (regardless presence of AS clause).
> 
> For instance, DB2 (which we consider to be the closest to ANSI):
> https://www.ibm.com/support/knowledgecenter/en/SSEPGG_9.7.0/com.ibm.db2.luw.apdv.java.doc/src/tpc/imjcc_c0052593.html

The example by the link just proves my words. Label is either
an identifier after AS, or the string before AS. This is always
defined. Name is a table column name. Regardless of whether
there is 'AS', and what is written after it.

But by the link above I don't see an example of SELECT of an
expression. As I understood from the examples Alexander showed
be, name is undefined for expressions.

> 
> The only exception is MimerSQL where both getColumnLabel() and
> getColumnName() always return alias (p.37 changes in 2.2):
> https://download.mimer.com/pub/developer/docs/latest_jdbcguide/mimjdben.pdf

Lol, what is mimersql? This is not our case.

We now return 'name' from box.execute in meta. This is
equivalent to what JDBC expects as 'label'.

Again. Mapping:

       Tarantool                   JDBC
   -------------------------------------------
    IPROTO_FIELD_NAME              Label  <- This one is defined and returned now.

  IPROTO_FIELD_<WHATEVER>          Name   <- This one is not defined now. And
                                             your patch is expected to add it.

Example:

    SELECT column1 AS alias, column2, column3 + 1, 100, 200 AS twohundred;

Result expected by JDBC:

    name = 'column1', label = 'alias';
    name = 'column2', label = 'column2';
           -        , label = 'column3 + 1';
           -        , label = 100;
           -        , label = 'twohundred';

Result we return now:

    IPROTO_FIELD_NAME = 'alias';
    IPROTO_FIELD_NAME = 'column2';
    IPROTO_FIELD_NAME = 'column3 + 1';
    IPROTO_FIELD_NAME = '100';
    IPROTO_FIELD_NAME = 'twohundred';

As you can see, we return JDBC's label now. We need
to add JDBC's name.

>> In our current implementation IPROTO_FIELD_NAME is in fact
>> 'label'. And we don't have 'name'. I think you need to keep
>> the IPROTO_FIELD_NAME as is, and add 'name' as the driver
>> expects it. For example, IPROTO_FIELD_ORIG_NAME. I.e. the
>> original column name for table columns. That will keep
>> backward compatibility, and will provide all the needed meta.
>>
>> You also need to get in touch with Alexander about that. I may
>> be wrong about the points above, and he knows more.
>>