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

[Nikita Pettik <korablev@tarantool.org>]

On 28 Nov 23:41, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> I see, that we calculate nullable and autoinc for
> table columns only. And it makes sense. Then maybe
> we need collations and alias also for columns only,
> not for all strings and result set columns? Could you
> please investigate? I think Alexander knows.

Alexander is guided by Oracle docs:
https://docs.oracle.com/javase/8/docs/api/java/sql/ParameterMetaData.html

Firstly, there's isCaseSensitive() method instead of getCollation().
However, collation includes information about case sensitivity, so
I've decided to include collation to metadata. Secodnly, from this
doc it's unclear whether isCaseSensitive() method applies to columns
which correspond to table columns, or to any column of result set
(and I've failed to find accurate description of result set in ANSI).
On the other hand, setting collation for expressions worth nothing, so
I see no reason (except a few additional bytes in netbox response) to
not include collation for all expressions.
To avoid overhead when user doesn't need extended meta, we can add
setting (pragma or whatever) like sql_extended_meta = true/false.

> On 27/11/2019 13:15, Nikita Pettik wrote:
> > If resulting set column is of STRING type and features collation (no
> > matter explicit or implicit) different from "none", then metadata will
> > contain its name.
> > 
> > Part of #4407
> > ---
> >  src/box/execute.c          |  31 +++++++--
> >  src/box/iproto_constants.h |   1 +
> >  src/box/lua/execute.c      |   7 +-
> >  src/box/lua/net_box.c      |  20 +++++-
> >  src/box/sql/select.c       |  18 ++++++
> >  src/box/sql/sqlInt.h       |   3 +
> >  src/box/sql/vdbe.h         |   4 ++
> >  src/box/sql/vdbeInt.h      |   1 +
> >  src/box/sql/vdbeapi.c      |   9 +++
> >  src/box/sql/vdbeaux.c      |  16 +++++
> >  test/sql/collation.result  | 155 +++++++++++++++++++++++++++------------------
> >  11 files changed, 195 insertions(+), 70 deletions(-)
> > 
> > diff --git a/src/box/execute.c b/src/box/execute.c
> > index e8b012e5b..20bfd0957 100644
> > --- a/src/box/execute.c
> > +++ b/src/box/execute.c
> > @@ -267,6 +267,23 @@ error:
> >  	region_truncate(region, svp);
> >  	return -1;
> >  }
> > +static size_t
> 
> 1. Please, add an empty line between these 2 functions.
> (In this commit.)

Fixed.
 
> > +/** Decode optional (i.e. may be present in response) metadata fields. */
> > +static void
> > +decode_metadata_optional(struct lua_State *L, const char **data,
> > +			 uint32_t map_size)
> > +{
> > +	/* 2 is default metadata map size (field name + field size). */
> > +	while (map_size-- > 2) {
> > +		uint32_t key = mp_decode_uint(data);
> > +		uint32_t len;
> > +		if (key == IPROTO_FIELD_COLL) {
> > +			const char *coll = mp_decode_str(data, &len);
> > +			lua_pushlstring(L, coll, len);
> > +			lua_setfield(L, -2, "collation");
> > +		}
> > +	}
> 
> 2. Netbox relies on certain order of fields in iproto
> response so as to avoid cycles and checking for already
> decoded fields. You can safely use it too. To eliminate
> the 'while' cycle you can append to netbox_decode_metadata's
> cycle:
> 
>     if (map_size == 2)
>         continue;
>     key = mp_decode_uint(data);
>     if (key == IPROTO_FIELD_COLL) {
>         /* handle coll ... */
>     }
>     if (map_size == 3)
>         continue;
>     key = mp_decode_uint(data);
>     if (key == IPROTO_FIELD_NULLABLE) {
>         /* handle nullable ... */
>     }
> 

TBO current implementation with cycle looks cleaner for me.
If you will excuse me, I leave it as it is.

> > +
> >  /**
> >   * Decode IPROTO_METADATA into array of maps.
> >   * @param L Lua stack to push result on.
> > diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> > index d6b8a158f..66e8c1274 100644
> > --- a/src/box/sql/select.c
> > +++ b/src/box/sql/select.c
> > @@ -1794,6 +1794,22 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> >  			var_pos[var_count++] = i;
> >  		vdbe_set_metadata_col_type(v, i,
> >  					   field_type_strs[sql_expr_type(p)]);
> > +		if (sql_expr_type(p) == FIELD_TYPE_STRING) {
> > +			bool unused;
> > +			uint32_t id;
> > +			struct coll *coll = NULL;
> > +			/*
> > +			 * If sql_expr_coll fails then it fails somewhere
> > +			 * above the call stack.
> > +			 */
> 
> 3. And lets add an assertion for that.

diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index d65644848..962f20d76 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1802,7 +1802,9 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
                         * If sql_expr_coll fails then it fails somewhere
                         * above the call stack.
                         */
-                       (void) sql_expr_coll(pParse, p, &unused, &id, &coll);
+                       int rc =  sql_expr_coll(pParse, p, &unused, &id, &coll);
+                       assert(rc == 0);
+                       (void) rc;
                        if (id != COLL_NONE) {
                                struct coll_id *coll_id = coll_by_id(id);
                                vdbe_set_metadata_col_collation(v, i,
 
> > +			(void) sql_expr_coll(pParse, p, &unused, &id, &coll);
> > +			if (id != COLL_NONE) {
> > +				struct coll_id *coll_id = coll_by_id(id);
> > +				vdbe_set_metadata_col_collation(v, i,
> > +								coll_id->name,
> > +								coll_id->name_len);
> > +			}
> > +		}
> >  		if (pEList->a[i].zName) {
> >  			char *zName = pEList->a[i].zName;
> >  			vdbe_set_metadata_col_name(v, i, zName);
> > @@ -1819,6 +1836,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> >  			} else {
> >  				vdbe_set_metadata_col_name(v, i, zCol);
> >  			}
> > +
> >  		} else {
> 
> 4. How about drop of these two hunks consisting of empty lines?

Sorry, removed.