[tarantool-patches] Re: [PATCH v3 9/9] sql: remove sqlErrorMsg()

• From: "n.pettik" <korablev@xxxxxxxxxxxxx>
• To: tarantool-patches@xxxxxxxxxxxxx
• Date: Tue, 5 Mar 2019 15:16:12 +0300

@@ -1284,10 +1302,13 @@ sql_create_view(struct Parse *parse_context, struct 
Token *begin,
              goto create_view_fail;
      if (aliases != NULL) {
              if ((int)select_res_space->def->field_count != aliases->nExpr) {
-                     sqlErrorMsg(parse_context, "expected %d columns "\
-                                     "for '%s' but got %d", aliases->nExpr,
-                                     space->def->name,
-                                     select_res_space->def->field_count);
+                     const char *err_msg =
+                             tt_sprintf("expected %d columns for '%s' but "\
+                                        "got %d", aliases->nExpr,
+                                        space->def->name,
+                                        select_res_space->def->field_count);
+                     diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);
+                     parse_context->is_aborted = true;
                      goto create_view_fail;
              }
              sqlColumnsFromExprList(parse_context, aliases, space->def);
@@ -1609,13 +1630,17 @@ sql_drop_table(struct Parse *parse_context, struct 
SrcList *table_name_list,
       * and DROP VIEW is not used on a table.
       */
      if (is_view && !space->def->opts.is_view) {
-             sqlErrorMsg(parse_context, "use DROP TABLE to delete table %s",
-                             space_name);
+             const char *err_msg = tt_sprintf("use DROP TABLE to delete "\
+                                              "table %s", space_name);
+             diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);

Why not ER_DROP_SPACE?

+             parse_context->is_aborted = true;
              goto exit_drop_table;
      }
      if (!is_view && space->def->opts.is_view) {
-             sqlErrorMsg(parse_context, "use DROP VIEW to delete view %s",
-                             space_name);
+             const char *err_msg = tt_sprintf("use DROP VIEW to delete "\
+                                              "view %s", space_name);
+             diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);

The same question.

+             parse_context->is_aborted = true;
              goto exit_drop_table;
      }
      /*
@@ -1760,8 +1785,10 @@ sql_create_foreign_key(struct Parse *parse_context, 
struct SrcList *child,
              }
      } else {
              if (parent_space->def->opts.is_view) {
-                     sqlErrorMsg(parse_context,
-                                     "referenced table can't be view");
+                     const char *err_msg = tt_sprintf("referenced table "\
+                                                      "can't be view");
+                     diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);

ER_CREATE_FK_CONSTRAINT

+                     parse_context->is_aborted = true;
                      goto exit_create_fk;
              }
      }
@@ -2149,7 +2176,9 @@ sql_create_index(struct Parse *parse, struct Token 
*token,
      struct space_def *def = space->def;

      if (def->opts.is_view) {
-             sqlErrorMsg(parse, "views can not be indexed");
+             const char *err_msg = tt_sprintf("views can not be indexed");
+             diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);

ER_CREATE_INDEX

+             parse->is_aborted = true;
              goto exit_create_index;
      }
      /*
@@ -2947,11 +2976,8 @@ sqlSavepoint(Parse * pParse, int op, Token * pName)
                      return;
              }
              if (op == SAVEPOINT_BEGIN &&
-                     sqlCheckIdentifierName(pParse, zName)
-                             != SQL_OK) {
-                     sqlErrorMsg(pParse, "bad savepoint name");
+                     sqlCheckIdentifierName(pParse, zName) != SQL_OK)

!= 0

                      return;
-             }
              sqlVdbeAddOp4(v, OP_Savepoint, op, 0, 0, zName, P4_DYNAMIC);
      }
}
@@ -1757,8 +1759,10 @@ sqlExprListAppendVector(Parse * pParse,        /* 
Parsing context */
       */
      if (pExpr->op != TK_SELECT
          && pColumns->nId != (n = sqlExprVectorSize(pExpr))) {
-             sqlErrorMsg(pParse, "%d columns assigned %d values",
-                             pColumns->nId, n);
+             const char *err_msg = tt_sprintf("%d columns assigned %d "\
+                                              "values", pColumns->nId, n);
+             diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);
+             pParse->is_aborted = true;
              goto vector_append_error;
      }

@@ -1878,7 +1882,10 @@ sqlExprListCheckLength(Parse * pParse,
      testcase(pEList && pEList->nExpr == mx);
      testcase(pEList && pEList->nExpr == mx + 1);
      if (pEList && pEList->nExpr > mx) {
-             sqlErrorMsg(pParse, "too many columns in %s", zObject);
+             const char *err_msg = tt_sprintf("too many columns in %s",
+                                              zObject);

You introduced LIMIT and “too many columns” errors in previous patch.
Why you can’t use them here?

+             diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);
+             pParse->is_aborted = true;
      }
}

diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index b69f059..3653824 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -897,7 +897,9 @@ expr(A) ::= VARIABLE(X).     {
  Token t = X;
  if (pParse->parse_only) {
    spanSet(&A, &t, &t);
-    sqlErrorMsg(pParse, "bindings are not allowed in DDL");
+    const char *err_msg = tt_sprintf("bindings are not allowed in DDL”);

Em, why can’t you inline this var?

diag_set(ClientError, ER_SQL_PARSER_GENERIC, "bindings are not allowed in DDL");

+    diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);
+    pParse->is_aborted = true;
    A.pExpr = NULL;
  } else if (!(X.z[0]=='#' && sqlIsdigit(X.z[1]))) {
    u32 n = X.n;

diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 02eca37..63a1b96 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -392,14 +392,25 @@ lookupName(Parse * pParse,      /* The parsing context 
*/
                                      pOrig = pEList->a[j].pExpr;
                                      if ((pNC->ncFlags & NC_AllowAgg) == 0
                                          && ExprHasProperty(pOrig, EP_Agg)) {
-                                             sqlErrorMsg(pParse,
-                                                             "misuse of 
aliased aggregate %s",
-                                                             zAs);
+                                             const char *err_msg =
+                                                     tt_sprintf("misuse of "\
+                                                                "aliased "\
+                                                                "aggregate "\
+                                                                "%s", zAs);

Such formatting looks terrible. Lets break 80 border or move
declaration of this error msg at few blocks above.

@@ -633,9 +647,13 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
                                                  exprProbability(pList->a[1].
                                                                  pExpr);
                                              if (pExpr->iTable < 0) {
-                                                     sqlErrorMsg(pParse,
-                                                                     "second 
argument to likelihood() must be a "
-                                                                     
"constant between 0.0 and 1.0");
+                                                     const char *err_msg =
+                                                             
tt_sprintf("second argument to likelihood() must be a "\
+                                                                        
"constant between 0.0 and 1.0”);

You don’t need sprintf here.

+                                                     diag_set(ClientError,
+                                                              
ER_SQL_PARSER_GENERIC,
+                                                              err_msg);
+                                                     pParse->is_aborted = 
true;
                                                      pNC->nErr++;
                                              }
                                      } else {
@@ -802,7 +827,11 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
                              testcase(pExpr->op == TK_GT);
                              testcase(pExpr->op == TK_GE);
                              testcase(pExpr->op == TK_BETWEEN);
-                             sqlErrorMsg(pParse, "row value misused");
+                             const char *err_msg = tt_sprintf("row value "\
+                                                              "misused”);

The same.

+                             diag_set(ClientError, ER_SQL_PARSER_GENERIC,
+                                      err_msg);
+                             pParse->is_aborted = true;
                      }
                      break;
              }
@@ -950,7 +964,10 @@ resolveCompoundOrderBy(Parse * pParse,   /* Parsing 
context.  Leave error messages
      db = pParse->db;
#if SQL_MAX_COLUMN
      if (pOrderBy->nExpr > db->aLimit[SQL_LIMIT_COLUMN]) {
-             sqlErrorMsg(pParse, "too many terms in ORDER BY clause");
+             const char *err_msg = tt_sprintf("too many terms in ORDER BY "\
+                                              "clause”);

The same.

+             diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);
+             pParse->is_aborted = true;
              return 1;
      }
#endif
@@ -1024,9 +1045,11 @@ resolveCompoundOrderBy(Parse * pParse, /* Parsing 
context.  Leave error messages
      }
      for (i = 0; i < pOrderBy->nExpr; i++) {
              if (pOrderBy->a[i].done == 0) {
-                     sqlErrorMsg(pParse,
-                                     "%r ORDER BY term does not match any "
-                                     "column in the result set", i + 1);
+                     const char *err_msg =
+                             tt_sprintf("ORDER BY term does not match any "\
+                                        "column in the result set”);

The same.

+                     diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);
+                     pParse->is_aborted = true;
                      return 1;
              }
      }
@@ -1417,9 +1450,15 @@ resolveSelectStep(Walker * pWalker, Select * p)
                      for (i = 0, pItem = pGroupBy->a; i < pGroupBy->nExpr;
                           i++, pItem++) {
                              if (ExprHasProperty(pItem->pExpr, EP_Agg)) {
-                                     sqlErrorMsg(pParse,
-                                                     "aggregate functions 
are not allowed in "
-                                                     "the GROUP BY clause");
+                                     const char *err_msg =
+                                             tt_sprintf("aggregate "\
+                                                        "functions are not "\
+                                                        "allowed in the "\
+                                                        "GROUP BY clause”);

Ok, verify all usages of sprintf and make sure they are really required.
I see several more in code.

@@ -613,8 +622,11 @@ sqlProcessJoin(Parse * pParse, Select * p)
              /* Disallow both ON and USING clauses in the same join
               */
              if (pRight->pOn && pRight->pUsing) {
-                     sqlErrorMsg(pParse, "cannot have both ON and USING "
-                                     "clauses in the same join");
+                     const char *err_msg =
+                             tt_sprintf("cannot have both ON and USING "
+                                        "clauses in the same join");
+                     diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);
+                     pParse->is_aborted = true;
                      return 1;
              }

@@ -650,10 +662,16 @@ sqlProcessJoin(Parse * pParse, Select * p)
                                  || !tableAndColumnIndex(pSrc, i + 1, zName,
                                                          &iLeft, &iLeftCol)
                                  ) {
-                                     sqlErrorMsg(pParse,
-                                                     "cannot join using 
column %s - column "
-                                                     "not present in both 
tables",
-                                                     zName);
+                                     const char *err_msg =
+                                             tt_sprintf("cannot join using "\
+                                                        "column %s - "\
+                                                        "column not "\
+                                                        "present in both "\
+                                                        "tables", zName);

Horrible formatting.

diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 85718e1..5a74c65 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3182,7 +3182,6 @@ void sqlTreeViewWith(TreeView *, const With *);
#endif

void sqlSetString(char **, sql *, const char *);
-void sqlErrorMsg(Parse *, const char *, ...);
void sqlDequote(char *);
void sqlNormalizeName(char *z);
void sqlTokenInit(Token *, char *);
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 2f1268a..297ba01 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -621,8 +621,11 @@ codeTriggerProgram(Parse * pParse,       /* The parser 
context */
      sqlSubProgramsRemaining--;

      if (sqlSubProgramsRemaining == 0) {
-             sqlErrorMsg(pParse,
-                             "Maximum number of chained trigger activations 
exceeded.");
+             const char *err_msg = tt_sprintf("Maximum number of chained "\
+                                              "trigger activations "\
+                                              "exceeded.”);

Please, make sure that it is valid error. And the rest of errors
I pointed in google doc file. If this is unreachable code, then
replace error with assert.

-/*
 * Convert an SQL-style quoted string into a normal string by removing
 * the quote characters.  The conversion is done in-place.  If the
 * input does not begin with a quote character, then this routine
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 9957f3a..d4fbf17 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -1222,7 +1222,9 @@ valueFromFunction(sql * db,     /* The database 
connection */
      pFunc->xSFunc(&ctx, nVal, apVal);
      if (ctx.isError) {
              rc = ctx.isError;
-             sqlErrorMsg(pCtx->pParse, "%s", sql_value_text(pVal));
+             diag_set(ClientError, ER_SQL_PARSER_GENERIC,
+                      sql_value_text(pVal));

What kind of error is it? Please, add reasonable error message,
not only text representation of value.

+             pCtx->pParse->is_aborted = true;
      } else {
              sql_value_apply_type(pVal, type);
              assert(rc == SQL_OK);
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 33885d0..e2c91e0 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -3914,7 +3914,9 @@ wherePathSolver(WhereInfo * pWInfo, LogEst nRowEst)
      }

      if (nFrom == 0) {
-             sqlErrorMsg(pParse, "no query solution");
+             const char *err_msg = tt_sprintf("no query solution”);

Same: this path seems to be unreachable.

diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
index e190ad7..16a31a8 100755
--- a/test/sql-tap/e_select1.test.lua
+++ b/test/sql-tap/e_select1.test.lua
@@ -1855,13 +1855,13 @@ test:do_catchsql_test(
    "e_select-8.7.1.1",
    "SELECT x FROM d1 UNION ALL SELECT a FROM d2 ORDER BY x*z",
    {
-        1, "1st ORDER BY term does not match any column in the result set"})
+        1, "ORDER BY term does not match any column in the result set”}

Why did you change error message?
I see quite a lot affected tests.

• Follow-Ups:
  • [tarantool-patches] Re: [PATCH v3 9/9] sql: remove sqlErrorMsg()
    • From: Konstantin Osipov

• References:
  • [tarantool-patches] [PATCH v3 0/9] sql: use diag_set() for errors in SQL
    • From: imeevma
  • [tarantool-patches] [PATCH v3 9/9] sql: remove sqlErrorMsg()
    • From: imeevma

Other related posts:

• » [tarantool-patches] [PATCH v3 9/9] sql: remove sqlErrorMsg()- imeevma
• » [tarantool-patches] Re: [PATCH v3 9/9] sql: remove sqlErrorMsg()- Konstantin Osipov
• » [tarantool-patches] Re: [PATCH v3 9/9] sql: remove sqlErrorMsg() - n.pettik
• » [tarantool-patches] Re: [PATCH v3 9/9] sql: remove sqlErrorMsg()- Konstantin Osipov

1. Home
2. tarantool-patches
3. Archive
4. 03-2019

---

• » Previous by Date
• » Next by Date
• » Related Posts
• » View list details
• » Manage your subscription

---