[tarantool-patches] Re: [PATCH] sql: replace instr() with position()

[n.pettik]

>> Why these conditions can’t be compatible?
>> What is the difference between SQL_FUNC_NEEDCOLL
>> and new flag? Does NEEDCOLL refer to returning value?
>> Please, comment this part.
> These conditions can’t be compatible because
> SQL_FUNC_NEEDCOLL and SQL_FUNC_ARG_COLL ask for
> different actions. SQL_FUNC_NEEDCOLL asks
> to find first argument with collation in
> the arguments expression list (from left
> to right) and use it inside function
> as a collation of the whole expression.
> SQL_FUNC_ARG_COLL asks to check if
> arguments collations are compatible
> and use the right one if they are.
> I would add comment to the code.
> 
> If we want to establish proper collation
> handling in all functions, we should
> probably use only one flag to do check
> and set the right collation.
> 
> I would merge these flags into one, OK?

Yep.

>>> 					bool unused;
>>> 					uint32_t id;
>>> 					if (sql_expr_coll(pParse,
>>> @@ -4064,6 +4065,37 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>>> 						return 0;
>>> 				}
>>> 			}
>>> +			if (pDef->funcFlags & SQL_FUNC_ARG_COLL) {
>>> +				assert(nFarg == 2);
>> 
>> Add comment to this assertion or better to the whole branch.
>> It is not obvious at first sight. Why only functions with two args…
>> For instance, there’s MAX function, which may take any number
>> of arguments. Collations are also vital for this function, since
>> they are used to compare strings.
>> 
>> What is more, I suggest to move this part in a separate patch,
>> since position is not the only function that must apply collations
>> on its operands. I don’t insist on dozens of tests on each built-in
>> function, just a few.
> So do you propose to make two patches: first with adding proper
> collation checking/setting in sqlExprCodeTarget() and another
> with adding proper collation usage in functions in func.c?
> Is it OK if I put them in one patch set?

Ok, you can try. Anyway, you can always squash patches.

>> 
>>> +				struct coll *colls[2] = {NULL};
>> 
>> I’d rather use explicit naming:
>> struct coll *lhs_coll = NULL;
>> struct coll *rhs_coll = NULL;
>> 
>> It makes code a bit longer, but on the other hand IMHO
>> makes it less prone to typos.
> But this function sqlExprCodeTarget() is called for expression
> list (of arguments). If we would use it for functions like MAX
> (as we want to) then I think an array should be used. Or just
> two loops to check every pair of arguments collations for
> compatibility.

You don’t need array: just go through arguments from left
to the right and compare on compatibility their collations.

>> 
>>> +				uint32_t colls_ids[2] = {0};
>>> +				bool is_forced[2] = {false};
>>> +				if (sql_expr_coll(pParse, pFarg->a[0].pExpr,
>>> +						  &is_forced[0], &colls_ids[0],
>>> +						  &colls[0]) != 0)
>>> +					return 0;
>>> +				if (sql_expr_coll(pParse, pFarg->a[1].pExpr,
>>> +						  &is_forced[1], &colls_ids[1],
>>> +						  &colls[1]) != 0)
>>> +					return 0;
>>> +
>>> +				uint32_t coll_id;
>>> +				if (collations_check_compatibility(colls_ids[0],
>>> +								   is_forced[0],
>>> +								   colls_ids[1],
>>> +								   is_forced[1],
>>> +								   &coll_id)
>>> +								   != 0) {
>>> +					pParse->rc = SQL_TARANTOOL_ERROR;
>>> +					pParse->nErr++;
>>> +					return 0;
>>> +				}
>>> +
>>> +				coll = (coll_id == colls_ids[0]) ? colls[0] :
>>> +								   colls[1];
>>> +				if (coll == NULL)
>>> +					coll = coll_by_id(COLL_NONE)->coll;
>> 
>> Why do we need this?
> Because if there is no collation at all among
> arguments, then the context would not have any
> collation as well. And sqlGetFuncCollSeq()
> would fail with assertion in this case, because
> it always expects some collations being set in context.

Hm, afair we added “none” collation to avoid such situations.
Will look at sql_expr_coll() and check if we can fix this.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20190315/77f633a7/attachment.html>