From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 7B9222ECA2 for ; Fri, 26 Oct 2018 11:21:27 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id pMMt92NPuECv for ; Fri, 26 Oct 2018 11:21:27 -0400 (EDT) Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 32A182EC52 for ; Fri, 26 Oct 2018 11:21:27 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\)) Subject: [tarantool-patches] Re: [PATCH 2/2] sql: remove GLOB from Tarantool From: Nikita Tatunov In-Reply-To: <20181021034805.alwhmkpz3fbopqjz@tkn_work_nb> Date: Fri, 26 Oct 2018 18:21:19 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <4607dc428909e96915e9f0984a7733a0890a3185.1534436836.git.n.tatunov@tarantool.org> <76466086-2a5f-8f12-cbc3-4ddf26e30fd9@tarantool.org> <32D1E5EA-EA21-4E4B-B5F5-80B6578BFBED@tarantool.org> <3f8354cb-4a47-c3de-164b-c776c792b991@tarantool.org> <090683CD-6F88-492D-AF48-631220A24E36@tarantool.org> <20181021034805.alwhmkpz3fbopqjz@tkn_work_nb> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Alexander Turenko Cc: tarantool-patches@freelists.org, korablev@tarantool.org Hello again, Alexander! Here are some comments / fixes to the review. Issues: https://github.com/tarantool/tarantool/issues/3334 https://github.com/tarantool/tarantool/issues/3251 https://github.com/tarantool/tarantool/issues/3572 Branch = https://github.com/tarantool/tarantool/tree/N_Tatunov/gh-3251-where-like-h= angs > On Oct 21, 2018, at 06:48, Alexander Turenko = wrote: >=20 > I don't have objections re this commit in general. Please, fix minor > comments below and proceed with Nikita P. >=20 > WBR, Alexander Turenko. >=20 >> - * Providing there are symbols in string s this >> - * macro returns UTF-8 code of character and >> - * promotes pointer to the next symbol in the string. >> - * Otherwise return code is SQL_END_OF_STRING. >> + * Providing there are symbols in string s this macro returns >> + * UTF-8 code of character and promotes pointer to the next >> + * symbol in the string. If s points to an invalid UTF-8 symbol >> + * return code is SQL_INVALID_UTF8_SYMBOL. If there're no symbols >> + * left in string s return code is SQL_END_OF_STRING. >=20 >> -/* >> - * Possible error returns from sql_utf8_pattern_compare() >> +/** >> + * Possible error returns from sql_utf8_pattern_compare(). >> */ >=20 >> - * '%' Matches any sequence of zero or more characters. >> + * '%' Matches any sequence of zero or more >> + * characters. >=20 >> - * Ec Where E is the "esc" character and c is any other >> - * character, including '%', '_', and esc, match >> - * exactly c. >> + * Ec Where E is the "esc" character and c is any >> + * other character, including '%', '_', and esc, >> + * match exactly c. >=20 >> - const char * pattern_end =3D pattern + strlen(pattern); >> - const char * string_end =3D string + strlen(string); >> + const char *pattern_end =3D pattern + strlen(pattern); >> + const char *string_end =3D string + strlen(string); >=20 > Please, amend these hunks into the first patch. The first patch = includes > code style fixes, so it would look more appropriate in it, eps. when = you > already touch the code in it. Done. >=20 >> +static const int case_insensitive_like =3D 1; >> +static const int case_sensitive_like =3D 0; >=20 > Are not they are mutually exclusive? Can we leave just one? >=20 > Why they are not defines? I guess it=E2=80=99s a bad idea since we need pointers to them in some = functions. e.g: int *is_like_ci; if (is_case_sensitive) is_like_ci =3D (void *)&case_sensitive_like; else is_like_ci =3D (void *)&case_insensitive_like; sqlite3CreateFunc(db, "LIKE", 2, 0, is_like_ci, likeFunc, 0, 0, = 0); sqlite3CreateFunc(db, "LIKE", 3, 0, is_like_ci, likeFunc, 0, 0, = 0); >=20 > /** .. */ inside functions should be /* */ (they are just comments and > they aren't documentation comments). It appears here ans there, so, > please, grep the intire patch for such things. Fixed. >=20 >> - return sql_utf8_pattern_compare(zGlobPattern, zString, = &globInfo, '['); >> + return sql_utf8_pattern_compare(zPattern, zStr, = case_sensitive_like, esc); >=20 >> - return sql_utf8_pattern_compare(zPattern, zStr, = &likeInfoNorm, esc); >> + return sql_utf8_pattern_compare(zPattern, zStr, = case_insensitive_like, esc); >=20 > Exceeds 80 symbols thank you, didn=E2=80=99t notice. >=20 >> + * @param is_case_sensitive whether like should be case sensitive >> + * or not. >=20 > Indent is strange. fixed. >=20 >> + int *is_like_ci; >> + if (is_case_sensitive) >> + is_like_ci =3D (int *)&case_sensitive_like; >> + else >> + is_like_ci =3D (int *)&case_insensitive_like; >=20 > Discarding const qualifier? It seems not being right thing to do. It = is > better to define is_like_ci pointer as const. It=E2=80=99s not possible due to so-called =E2=80=9Cdown = qualification=E2=80=9D and will cause compiler warnings. >=20 >> -int sqlite3IsLikeFunction(sqlite3 *, Expr *, int *, char *); >> +int sql_is_like_func(sqlite3 *db, Expr *pExpr, int = *is_case_insensitive); >=20 > The code around does not have parameter names, so here they are looks > strange. fixed. >=20 >> + /* String on RHS of LIKE operator */ >> + const char *z =3D 0; >> + /* Right and left size of LIKE operator */ >> + Expr *pRight, *pLeft; >> + /* List of operands to the LIKE operator */ >> + ExprList *pList; >> + /* One character in z[] */ >> + int c; >> + /* Number of non-wildcard prefix characters */ >> + int cnt; >> + /* Database connection */ >> + sqlite3 *db =3D pParse->db; >> sqlite3_value *pVal =3D 0; >> - int op; /* Opcode of pRight */ >> - int rc; /* Result code to return */ >> + /* Opcode of pRight */ >> + int op; >> + /* Result code to return */ >> + int rc; >=20 > Our code style suggests to add a period at end of the comment. The = same > in exprAnalyze() below. Thank you, fixed. >=20 >> + INSERT INTO t1 SELECT a+5,6,'six'||substr(c,4) FROM t1 WHERE = c LIKE 'one-%'; >=20 > Trailing whitespaces. deleted. >=20 >> --- NOTE: This test needs refactoring after deletion of GLOB & >> --- type restrictions for LIKE. (See #3572) >> -- Another NOTE: MATCH & REGEXP aren't supported in Tarantool & >> --- are waiting for their hour, don't confuse them >> --- being commented with ticket above. >> - {"=3D", "=3D=3D", "!=3D", "<>"}, --"LIKE", "GLOB"}, --"MATCH", = "REGEXP"}, >> +-- are waiting for their hour, don't confuse them >> +-- being commented with commenting of "LIKE". >> + {"=3D", "=3D=3D", "!=3D", "<>"}, --"MATCH", "REGEXP"}, >=20 > Another NOTE -> NOTE. >=20 > 'don't confuse them being commented with commenting of "LIKE"' -- can = be > removed. Fixed. >=20 > test/sql-tap/like3.test.lua cases can be rewritten with LIKE instead = of > removing, right? I think it worth to do if it is so. Ok, sure.