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 088EB2CC82 for ; Sat, 20 Oct 2018 23:48:09 -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 xRlMKfe1bolg for ; Sat, 20 Oct 2018 23:48:08 -0400 (EDT) Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (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 451C02C7E6 for ; Sat, 20 Oct 2018 23:48:08 -0400 (EDT) Date: Sun, 21 Oct 2018 06:48:05 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH 2/2] sql: remove GLOB from Tarantool Message-ID: <20181021034805.alwhmkpz3fbopqjz@tkn_work_nb> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <090683CD-6F88-492D-AF48-631220A24E36@tarantool.org> 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: Nikita Tatunov Cc: tarantool-patches@freelists.org, korablev@tarantool.org I don't have objections re this commit in general. Please, fix minor comments below and proceed with Nikita P. WBR, Alexander Turenko. > - * 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. > -/* > - * Possible error returns from sql_utf8_pattern_compare() > +/** > + * Possible error returns from sql_utf8_pattern_compare(). > */ > - * '%' Matches any sequence of zero or more characters. > + * '%' Matches any sequence of zero or more > + * characters. > - * 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. > - const char * pattern_end = pattern + strlen(pattern); > - const char * string_end = string + strlen(string); > + const char *pattern_end = pattern + strlen(pattern); > + const char *string_end = string + strlen(string); 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. > +static const int case_insensitive_like = 1; > +static const int case_sensitive_like = 0; Are not they are mutually exclusive? Can we leave just one? Why they are not defines? /** .. */ 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. > - return sql_utf8_pattern_compare(zGlobPattern, zString, &globInfo, '['); > + return sql_utf8_pattern_compare(zPattern, zStr, case_sensitive_like, esc); > - return sql_utf8_pattern_compare(zPattern, zStr, &likeInfoNorm, esc); > + return sql_utf8_pattern_compare(zPattern, zStr, case_insensitive_like, esc); Exceeds 80 symbols > + * @param is_case_sensitive whether like should be case sensitive > + * or not. Indent is strange. > + int *is_like_ci; > + if (is_case_sensitive) > + is_like_ci = (int *)&case_sensitive_like; > + else > + is_like_ci = (int *)&case_insensitive_like; Discarding const qualifier? It seems not being right thing to do. It is better to define is_like_ci pointer as const. > -int sqlite3IsLikeFunction(sqlite3 *, Expr *, int *, char *); > +int sql_is_like_func(sqlite3 *db, Expr *pExpr, int *is_case_insensitive); The code around does not have parameter names, so here they are looks strange. > + /* String on RHS of LIKE operator */ > + const char *z = 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 = pParse->db; > sqlite3_value *pVal = 0; > - int op; /* Opcode of pRight */ > - int rc; /* Result code to return */ > + /* Opcode of pRight */ > + int op; > + /* Result code to return */ > + int rc; Our code style suggests to add a period at end of the comment. The same in exprAnalyze() below. > + INSERT INTO t1 SELECT a+5,6,'six'||substr(c,4) FROM t1 WHERE c LIKE 'one-%'; Trailing whitespaces. > --- 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. > - {"=", "==", "!=", "<>"}, --"LIKE", "GLOB"}, --"MATCH", "REGEXP"}, > +-- are waiting for their hour, don't confuse them > +-- being commented with commenting of "LIKE". > + {"=", "==", "!=", "<>"}, --"MATCH", "REGEXP"}, Another NOTE -> NOTE. 'don't confuse them being commented with commenting of "LIKE"' -- can be removed. 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.