[patches] [PATCH 0/7] iproto: send SQL column meta on SELECT

[v.shpilevoy at tarantool.org]

> 1 марта 2018 г., в 14:47, n.pettik <korablev at tarantool.org> написал(а):
> 
> Hello. See few minor comments below.
> (I don’t want to spam, so I put all comments together in one letter)
> 
> [PATCH 1/7] collation: introduce coll_is_case_sensitive function
> 
>> diff --git a/src/box/coll.h b/src/box/coll.h
>> index aa0e4c01a..f171e582c 100644
>> --- a/src/box/coll.h
>> +++ b/src/box/coll.h
>> @@ -78,6 +78,14 @@ 
>> +/**
>> + * Return true, if a collation is case sensitive.
>> + * @param coll Collation to check.
>> + * @retval Case sensitivity.
>> + */
>> +bool
>> +coll_is_case_sensitive(const struct coll *coll);
> 
> Not sure that this comment is really needed. I mean, function's name completely
> describes what it does.

Commenting each function is part of our code style.

> 
> ///////////////////////////////////////////////////////////////////////
> 
> [PATCH 2/7] sql: remove SQLITE_ENABLE_COLUMN_METADATA and _OMIT_DECLTYPE
> 
>> 
>> 		if (db->mallocFailed) {
>> 			sqlite3OomClear(db);
>> -			ret = 0;
>> +			ret = NULL;
> 
> Here you are patching function which you anyway remove in next commit.
> I think it is redundant diff, since these commits are placed in one patch-set.

Fixed.

> 
> ///////////////////////////////////////////////////////////////////////
> 
> [PATCH 5/7] sql: store column meta in a special structure instead of Mem
> 
>> 
>> +		uint32_t size = sizeof(p->columns[0]) * n;
>> +		struct sql_column_meta *new_meta = (struct sql_column_meta *)
>> +			sqlite3DbRealloc(db, p->columns, size);
>> +		if (new_meta == NULL)
>> +			return;
> 
> I suggest to indicate somehow that sqlite3DbRealloc has failed
> (even if it occurs extremely rarely).
> For instace, by setting sqlite3Error().

sqlite3DbRealloc do it inside, like all sqlite3Malloc... functions. It is the single reason, why I use it here instead of malloc/realloc/free.

> 
>> 
>> +		} else {
> 
> I would say it is too gently to silently skip situation when collation is missed.
> I can't imagine reason when collation isn't found by name, since AFAIK in SQL every
> column features collation (even if it wasn't set, by default it would be "BINARY").

Try to write assert(coll != NULL), and sql-tap/collation.test.lua fails, because BINARY collation in SQL has name, that can not be found in Tarantool.

> And why do you set case sensitivity to be true if there is no found collation?
> Yep, there are only two variants what to do (case/nocase), but anyway
> this moment deserves to be mentioned/commented somewhere.
> 
>> +		if (column->zName == alias) {
>> +			/*
>> +			 * If a column is selected with no alias,
>> +			 * then do not copy it twice. Store two
>> +			 * pointers to the same name.
>> +			 */
>> +			destructor = SQLITE_STATIC;
>> +			alias = meta->name;
> 
> I suppose it should be strcmp() instead of == ?

Fixed.

> 
>> @@ -1695,51 +1697,23 @@ columnType(NameContext *pNC, Expr *pExpr, const char **pzOrigTab,
>> -	if (pzOrigTab) {
>> -		assert(pzOrigTab && pzOrigCol);
>> -		*pzOrigTab = zOrigTab;
>> -		*pzOrigCol = zOrigCol;
>> +	if (table != NULL) {
>> +		assert(table != NULL && fieldno != NULL);
> 
> LHS condition in assert is redundant: if you get in this branch, table is already != NULL.

Fixed.

> 
> ///////////////////////////////////////////////////////////////////////
> 
> [PATCH 6/7] iproto: send SQL column meta on SELECT
> 
> Also, I agree with Kirill that function’s name ‘sql_ephemeral_column_meta_encode’ can be confusing.
> I can suggest synonyms: surrogate, pseudo, dummy, synthetic/artificial.

I prefer 'artificial'.

>