From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Imeev Mergen <imeevma@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v3 4/9] sql: remove field rc of struct Parse Date: Tue, 5 Mar 2019 12:06:38 +0300 [thread overview] Message-ID: <D8453DE4-529C-457F-9AA7-A3048CDBF39E@tarantool.org> (raw) In-Reply-To: <50846f935fdc69a4ff17958b8307bc9dc245fe70.1551530224.git.imeevma@gmail.com> Nit: remove from. > Currently, the field rc of the struct Parse can have only two > values: SQL_OK or SQL_TARANTOOL_ERROR. Therefore, it is logical to > replace it with a new boolean field. This patche replaces field rc > by new field is_aborted. Fixed commit message (original one contained several mistakes): sql: replace rc with is_abort status in stuct Parse Currently, field representing return code in struct Parse can take only two values: SQL_OK (successfully finished parsing) and SQL_TARANTOOL_ERROR (in case of any errors occurred). Therefore, it can be replaced with a boolean field. Patch provides straightforward refactoring. Part of #3965 > diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c > index 0c6296d..42737ff 100644 > --- a/src/box/sql/prepare.c > +++ b/src/box/sql/prepare.c > @@ -100,15 +100,15 @@ sqlPrepare(sql * db, /* Database handle. */ > } > assert(0 == sParse.nQueryLoop); > > - if (sParse.rc == SQL_DONE) > - sParse.rc = SQL_OK; > if (db->mallocFailed) { > - sParse.rc = SQL_NOMEM; > + diag_set(OutOfMemory, 0, "SQL", "db"); > + sParse.is_aborted = true; See comments for previous patches. > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 719b85e..42ff4b8 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -2643,7 +2643,6 @@ struct Parse { > sql *db; /* The main database structure */ > char *zErrMsg; /* An error message */ > Vdbe *pVdbe; /* An engine for executing database bytecode */ > - int rc; /* Return code from execution */ > u8 colNamesSet; /* TRUE after OP_ColumnName has been issued to pVdbe */ > u8 nTempReg; /* Number of temporary registers in aTempReg[] */ > u8 isMultiWrite; /* True if statement may modify/insert multiple rows */ > @@ -2677,6 +2676,8 @@ struct Parse { > u8 eOrconf; /* Default ON CONFLICT policy for trigger steps */ > /** Region to make SQL temp allocations. */ > struct region region; > + /** Flag to show that parsing should be aborted. */ Comment is misleading: now we don’t abort parsing process, but instead allow it to be finished and raise an error at the end. Fix comment pls. > + bool is_aborted; > > /************************************************************************** > * Fields above must be initialized to zero. The fields that follow, > @@ -534,25 +533,17 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg) > sqlParserFree(pEngine, sql_free); > if (db->mallocFailed) { > diag_set(OutOfMemory, 0, "SQL", "db"); > - pParse->rc = SQL_TARANTOOL_ERROR; > - } > - if (pParse->rc != SQL_OK && pParse->rc != SQL_DONE > - && pParse->zErrMsg == 0) { > - const char *error; > - if (is_tarantool_error(pParse->rc) && > - tarantoolErrorMessage() != NULL) > - error = tarantoolErrorMessage(); > - else > - error = sqlErrStr(pParse->rc); > - pParse->zErrMsg = sqlMPrintf(db, "%s", error); > + pParse->is_aborted = true; > } > + if (pParse->is_aborted && pParse->zErrMsg == 0) > + pParse->zErrMsg = sqlMPrintf(db, "%s", tarantoolErrorMessage()); > assert(pzErrMsg != 0); > if (pParse->zErrMsg) { > *pzErrMsg = pParse->zErrMsg; > - sql_log(pParse->rc, "%s", *pzErrMsg); > + sql_log(SQL_TARANTOOL_ERROR, "%s", *pzErrMsg); Do we need this call at all? > diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua > index bab6493..0d8bf15 100755 > --- a/test/sql-tap/check.test.lua > +++ b/test/sql-tap/check.test.lua > @@ -516,7 +516,7 @@ test:do_catchsql_test( > ); > ]], { > -- <check-5.1> > - 1, "Wrong space options (field 5): invalid expression specified (SQL error: bindings are not allowed in DDL)" > + 1, "Wrong space options (field 5): invalid expression specified (bindings are not allowed in DDL)" > -- </check-5.1> > }) Why test results have changed if you provided non-functional refactoring?
next prev parent reply other threads:[~2019-03-05 9:06 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-03-02 13:07 [tarantool-patches] [PATCH v3 0/9] sql: use diag_set() for errors in SQL imeevma 2019-03-02 13:07 ` [tarantool-patches] [PATCH v3 1/9] sql: rework syntax errors imeevma 2019-03-04 17:47 ` [tarantool-patches] " n.pettik 2019-03-05 8:31 ` Konstantin Osipov 2019-03-02 13:07 ` [tarantool-patches] [PATCH v3 2/9] sql: save SQL parser errors in diag_set() imeevma 2019-03-05 8:40 ` [tarantool-patches] " Konstantin Osipov 2019-03-05 9:06 ` n.pettik 2019-03-02 13:07 ` [tarantool-patches] [PATCH v3 3/9] sql: remove field nErr of struct Parse imeevma 2019-03-05 8:41 ` [tarantool-patches] " Konstantin Osipov 2019-03-05 9:06 ` n.pettik 2019-03-02 13:07 ` [tarantool-patches] [PATCH v3 4/9] sql: remove field rc " imeevma 2019-03-05 8:42 ` [tarantool-patches] " Konstantin Osipov 2019-03-05 9:06 ` n.pettik [this message] 2019-03-02 13:07 ` [tarantool-patches] [PATCH v3 5/9] sql: remove field zErrMsg " imeevma 2019-03-05 8:43 ` [tarantool-patches] " Konstantin Osipov 2019-03-05 9:06 ` n.pettik 2019-03-02 13:07 ` [tarantool-patches] [PATCH v3 6/9] sql: rework six syntax errors imeevma 2019-03-05 8:45 ` [tarantool-patches] " Konstantin Osipov 2019-03-05 9:07 ` n.pettik 2019-03-02 13:08 ` [tarantool-patches] [PATCH v3 7/9] sql: rework four semantic errors imeevma 2019-03-05 8:46 ` [tarantool-patches] " Konstantin Osipov 2019-03-05 9:16 ` n.pettik 2019-03-02 13:08 ` [tarantool-patches] [PATCH v3 8/9] sql: rework three errors of "unsupported" type imeevma 2019-03-05 8:47 ` [tarantool-patches] " Konstantin Osipov 2019-03-05 9:34 ` n.pettik 2019-03-05 9:43 ` Konstantin Osipov 2019-03-02 13:08 ` [tarantool-patches] [PATCH v3 9/9] sql: remove sqlErrorMsg() imeevma 2019-03-05 8:48 ` [tarantool-patches] " Konstantin Osipov 2019-03-05 12:16 ` n.pettik 2019-03-05 15:44 ` Konstantin Osipov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=D8453DE4-529C-457F-9AA7-A3048CDBF39E@tarantool.org \ --to=korablev@tarantool.org \ --cc=imeevma@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v3 4/9] sql: remove field rc of struct Parse' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox