Re: [Tarantool-patches] [PATCH v5 2/5] sql: remove PRAGMA "short_column_names"

[Mergen Imeev <imeevma@tarantool.org>]

Hi! Thank you for remarks. My answers below.

On Fri, Dec 27, 2019 at 06:55:22PM +0200, Nikita Pettik wrote:
> On 27 Dec 14:18, imeevma@tarantool.org wrote:
> > The pragmas "short_column_names" and "full_column_names" allow us
> > to use three ways to display the column name in metadata:
> > 1) If both are turned off, then the column name was shown as it
> > was written by the user.
> > 2) If "short_column_names" = OFF and "full_column_names" = ON,
> > then the column name is displayed as <table name>.<column name>.
> > 3) If "short_column_names" = ON, then the column name is displayed
> > as <column name>. This is the default option.
> 
> Don't get the difference between first and last options. Also see
> https://www.sqlite.org/pragma.html#pragma_full_column_names
>  
Here you can see behaiour in the first case:

tarantool> CREATE TABLE t(i INT PRIMARY KEY);
---
- row_count: 1
...

tarantool> SELECT t     .        i from t;
---
- metadata:
  - name: t     .        i
    type: integer
  rows: []
...

As we can see, the name is "t     .        i". In the third
case:

tarantool> SELECT t     .        i from t;
---
- metadata:
  - name: I
    type: integer
  rows: []
...


> > But we need only two ways to show the column name:
> > 1) Show the column name as <column name>. This should be the
> > default option.
> > 2) Show the column name as <table name>.<column name>.
> > 
> > In this regard, we need only one of these pragmas.
> > 
> > @@ -1827,12 +1824,11 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> >  			assert(iCol >= 0 && iCol < (int)space_def->field_count);
> >  			zCol = space_def->fields[iCol].name;
> >  			const char *name = NULL;
> > +
> >  
> 
> Extra new line. Please, remove.
> 
Fixed.

> >			if (pEList->a[i].zName != NULL) {
> >  				name = pEList->a[i].zName;
> >  			} else {
> > -				if (!shortNames && !fullNames) {
> > -					name = pEList->a[i].zSpan;
> > -				} else if (fullNames) {
> > +				if (pParse->sql_flags & SQL_FullColNames) {
> 
> (pParse->sql_flags & SQL_FullColNames) != 0
> 
Fixed.

> >  					name = tt_sprintf("%s.%s",
> >  							  space_def->name,
> >  							  zCol);
> > @@ -2053,8 +2049,7 @@ sqlResultSetOfSelect(Parse * pParse, Select * pSelect)
> >  	sql *db = pParse->db;
> >  
> >  	uint32_t saved_flags = pParse->sql_flags;
> > -	pParse->sql_flags |= ~SQL_FullColNames;
> > -	pParse->sql_flags &= SQL_ShortColNames;
> > +	pParse->sql_flags = 0;
> 
> Why do you set sql_flags to zero?
> 
After "pParse->sql_flags &= SQL_ShortColNames;" all but
one(SQL_ShortColNames) bits will be 0. Since we remove
SQL_ShortColNames, all bits will be 0.

> >  	sqlSelectPrep(pParse, pSelect, 0);
> >  	if (pParse->is_aborted)
> >  		return NULL;
> 
> I comment your patch since I've faced the same necessity to remove
> short_column_names. So to avoid doubling work, I'am going to cherry-
> pick your patch (or push it to the master out of order).
>