[tarantool-patches] Re: [PATCH v4 8/8] sql: remove sqlErrorMsg()

["n.pettik" <korablev@tarantool.org>]

>> @@ -1039,8 +1036,8 @@ resolveCompoundOrderBy(Parse * pParse,    /* Parsing context.  Leave error messages
>>                        const char *err = "Error at ORDER BY in place %d: "\
>>                                          "term does not match any column in "\
>>                                          "the result set";
>> -                       err = tt_sprintf(err, i + 1);
>> -                       diag_set(ClientError, ER_SQL_PARSER_GENERIC, err);
>> +                       diag_set(ClientError, ER_SQL_PARSER_GENERIC,
>> +                                tt_sprintf(err, i + 1));
>> 
>>> @@ -1444,10 +1469,37 @@ resolveSelectStep(Walker * pWalker, Select * p)
>>>    * number of expressions in the select list.
>>>    */
>>>   if (p->pNext && p->pEList->nExpr != p->pNext->pEList->nExpr) {
>>> -     sqlSelectWrongNumTermsError(pParse, p->pNext);
>>> +     if (p->pNext->selFlags & SF_Values) {
>>> +       diag_set(ClientError, ER_SQL_PARSER_GENERIC,
>>> +          "all VALUES must have the same "\
>>> +          "number of terms");
>>> +     } else {
>>> +       const char *err_msg =
>>> +         "SELECTs to the left and right of %s "\
>>> +         "do not have the same number of "\
>>> +         "result columns";
>>> +       switch (p->pNext->op) {
>>> +       case TK_ALL:
>>> +         err_msg = tt_sprintf(err_msg,
>>> +                  "UNION ALL");
>>> +         break;
>> 
>> Why don’t use selectOpName?
>> Like it was in  sqlSelectWrongNumTermsError().
>> 
> Function selectOpName defined in different file and is static.
> I think it isn't worth to share it for one error.

I believe it is worth doing. For instance, you can return
sqlSelectWrongNumTermsError() which was placed in select.c
and called that static func.

> Diff between versions:
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index e26596a..1077285 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -666,13 +666,7 @@ codeVectorCompare(Parse * pParse,	/* Code generator context */
> 	int regRight = 0;
> 	u8 op = pExpr->op;
> 	int addrDone = sqlVdbeMakeLabel(v);
> -
> -	if (nLeft != sqlExprVectorSize(pRight)) {
> -		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> -			 "row value misused");
> -		pParse->is_aborted = true;
> -		return;
> -	}

Here the problem is that function is never called in our test suite:
I placed assert(0); and no one test failed. Please, add at least
simple tests like SELECT (1, 2) == (1, 2) to make sure that
codeVectorCompare() can be processed. Tests which you’ve added
fails before this func is called.

Added comment:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 107728590..397f8209c 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -666,6 +666,11 @@ codeVectorCompare(Parse * pParse,  /* Code generator context */
        int regRight = 0;
        u8 op = pExpr->op;
        int addrDone = sqlVdbeMakeLabel(v);
+       /*
+        * Situation when vectors have different dimensions is
+        * filtred way before - during expr resolution:
+        * see resolveExprStep().
+        */

> +	assert(nLeft == sqlExprVectorSize(pRight));
> 	assert(pExpr->op == TK_EQ || pExpr->op == TK_NE
> 	       || pExpr->op == TK_LT || pExpr->op == TK_GT
> 	       || pExpr->op == TK_LE || pExpr->op == TK_GE);
> 
> 
> @@ -4384,10 +4378,8 @@ sqlWhereBegin(Parse * pParse,	/* The parser context */
> 	 * equal to pTabList->nSrc but might be shortened to 1 if the
> 	 * WHERE_OR_SUBCLAUSE flag is set.
> 	 */
> -	for (ii = 0; ii < pTabList->nSrc; ii++) {
> +	for (ii = 0; ii < pTabList->nSrc; ii++)
> 		createMask(pMaskSet, pTabList->a[ii].iCursor);
> -		sqlWhereTabFuncArgs(pParse, &pTabList->a[ii], &pWInfo->sWC);

Move removal of this func to a separate patch pls alongside with mentions of
table-valued funcs. It is not related to errors.