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 A7CB527FBF for ; Fri, 27 Jul 2018 16:22:55 -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 Wn-rz3TI0tLi for ; Fri, 27 Jul 2018 16:22:55 -0400 (EDT) Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (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 CFBFD2821F for ; Fri, 27 Jul 2018 16:22:22 -0400 (EDT) Date: Fri, 27 Jul 2018 23:22:19 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] sql: LIKE & GLOB pattern comparison issue Message-ID: <20180727202219.ikwbax7tysfnmgr4@tkn_work_nb> References: <1530190036-10105-1-git-send-email-hollow653@gmail.com> <20180718024314.be245cmsgklxuvnk@tkn_work_nb> <20180727130601.b2oby7dleapd5upg@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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 Minor comments are below. Looks good for me in general. Please, review the patch with Lesha after the fixes. WBR, Alexander Turenko. > > > With the patch applied an error will be returned in case there's > > > invalid UTF-8 symbol in pattern & pattern will not be matched > > > with the string that contains it. Some minor corrections to function > > > were made as well. > > > > > > > Nitpicking: 'error will be returned' is third situatuon, other then > > 'matched' and 'not matched'. > > > > > I guess commit message is about changes. Do I really need to > mention valid cases that didn't change? > Ok, now I got that 'it' is 'invalid symbol'. Misunderstood it before as description of the one case when a pattern has invalid UTF-8 symbols. The sentence is correct. Maybe it can be clarified like so: '& correct UTF-8 pattern will not be matched with a string with invalid UTF-8 symbols'. Or in another way you like. 'Some minor corrections to function were made as well' -- describe certain behaviour or code changes (like 'fixed infinite loop during pattern matching and incorrect matching as well') or remove this abstract sentence. > > The function itself looks good except lines longer then 80 columns. > > > > As i said before I don't think breaking these lines will increase > readability. > There are two little length exceed place in the code, it needs some refactoring to decrease code blocks nesting level. I think we can lease it as is for now (if other reviewer don't ask for that). But comments within the function are worth to fix to fit the limit. > +test:do_catchsql_set_test(like_test_cases, prefix) > + > +-- Invalid testcases. > + > +for i, tested_string in ipairs(invalid_testcases) do > + > +-- We should raise an error if pattern contains invalid characters. Again, indent comments to the same level as code in the block. If a comment is related to several code blocks (with empty lines btw), then separate it with the empty line from the code (to don't misinterpret it as related to the first block). > + local test_name = prefix .. "2." .. tostring(i) > + local test_itself = "SELECT 'abc' LIKE 'ab" .. tested_string .. "';" > + test:do_catchsql_test( > + test_name, > + test_itself, { > + -- > + 1, "LIKE or GLOB pattern can only contain UTF-8 characters" > + -- > + }) > + Are there reason to format it like so? Maybe just: test:do_catchsql_test(test_name, test_itself, {1, "LIKE or GLOB pattern can only contain UTF-8 characters"})