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 795912F90F for ; Thu, 15 Nov 2018 06:04:11 -0500 (EST) 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 xImZoj4Xo2Xt for ; Thu, 15 Nov 2018 06:04:11 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 BBE402F902 for ; Thu, 15 Nov 2018 06:04:10 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 2/3] Introduce "none" and "binary" collations References: <8e9bca5d0f620a5cbd3bdffe521cecdfd1ad0ffd.1542066357.git.korablev@tarantool.org> <03A13ECD-6BA4-448F-8952-EE8E5C753130@tarantool.org> <9DBC0637-55A0-47E7-815F-CEA5FA1189AA@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 15 Nov 2018 14:04:05 +0300 MIME-Version: 1.0 In-Reply-To: <9DBC0637-55A0-47E7-815F-CEA5FA1189AA@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: "n.pettik" , tarantool-patches@freelists.org Hi! Thanks for the fixes! >>> In other words, each field with type of STRING/SCALAR and mb ANY >>> would have real pointer to collation. However, in turn such change >>> would break too many tests, without significant refactoring of SQL >>> query planner (at least I failed to do it in several hours). >>> On the other hand, in this case we can remove COLL_NONE >>> and unify collation interface: each field or key part MUST feature >>> collation. And I mean not only valid ID (it is how it works now), >>> but also valid (i.e. != NULL) pointer to collation struct. So, it seems >>> that we would be able to remove any checks like: >>> if (coll != NULL) >>> *use collation* >>> else >>> *process without involving collations* >>> Would become: >>> If (type_is_string_like) >>> *always use collation* >>> To be honest I was sure that you would notice this pickle… >>> Or current approach is not dat bad? >> >> Yeah, I understand your aspiration, but I have some remarks >> about the draft patch above and about the concept as it is: >> >> * you said that you want to get rid of COLL_NONE, but in the draft >> above you still use it. Even if you had removed COLL_NONE >> usage from key_def builders, it would have stayed in alter.cc >> as a check that you can not drop 'none' collation. I think that >> COLL_NONE as a constant equal to 0 should not be removed from >> source code until it is not used at all. But it is a minor >> comment rather about coding style: I prefer code like >> 'id != UNDERSTANDABLE_NAME' to 'id != '. > > And still we use iid != 0 to check whether index primary or not… Decent, but I do not like it too. I tried to convince Kostja that it is a bad pattern, but as usually he did not listen. > diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua > index 47fb7a809..1e8f096e1 100755 > --- a/test/sql-tap/e_select1.test.lua > +++ b/test/sql-tap/e_select1.test.lua > @@ -508,11 +508,11 @@ if (0 > 0) > test:do_execsql_test( > "e_select-1.6.0", > [[ > - CREATE TABLE t5(a TEXT COLLATE "unicode_ci", b TEXT COLLATE binary); > + CREATE TABLE t5(a TEXT COLLATE "unicode_ci", b TEXT COLLATE "binary"); > INSERT INTO t5 VALUES('AA', 'cc'); > INSERT INTO t5 VALUES('BB', 'dd'); > INSERT INTO t5 VALUES(NULL, NULL); > - CREATE TABLE t6(a TEXT COLLATE binary, b TEXT COLLATE "unicode_ci"); > + CREATE TABLE t6(a TEXT COLLATE "c", b TEXT COLLATE "unicode_ci"); Typo - binary turned into "c". I've force pushed a fix so as to allow Kirill Y. merge the branch as soon as possible. Nevertheless, I've put the fix at the end of the email, according to SOP. > INSERT INTO t6 VALUES('aa', 'cc'); > INSERT INTO t6 VALUES('bb', 'DD'); > INSERT INTO t6 VALUES(NULL, NULL); Also in the patch 'Introduce "none" and "binary" collations' I see one of COLL_NONE substituted back to 0. I have force fixed it too, and put at the end of the email. ================================================================== Fix for "sql: don't uppercase name of binary collation". diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua index 1e8f096e1..e1d814f47 100755 --- a/test/sql-tap/e_select1.test.lua +++ b/test/sql-tap/e_select1.test.lua @@ -512,7 +512,7 @@ if (0 > 0) INSERT INTO t5 VALUES('AA', 'cc'); INSERT INTO t5 VALUES('BB', 'dd'); INSERT INTO t5 VALUES(NULL, NULL); - CREATE TABLE t6(a TEXT COLLATE "c", b TEXT COLLATE "unicode_ci"); + CREATE TABLE t6(a TEXT COLLATE "binary", b TEXT COLLATE "unicode_ci"); INSERT INTO t6 VALUES('aa', 'cc'); INSERT INTO t6 VALUES('bb', 'DD'); INSERT INTO t6 VALUES(NULL, NULL); =================================================================== Fix for "Introduce "none" and "binary" collations". diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index 8fc217f68..5bdf102e7 100644 --- a/src/box/tuple_format.c +++ b/src/box/tuple_format.c @@ -40,7 +40,7 @@ static uint32_t formats_size = 0, formats_capacity = 0; static const struct tuple_field tuple_field_default = { FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false, - ON_CONFLICT_ACTION_NONE, NULL, 0, + ON_CONFLICT_ACTION_NONE, NULL, COLL_NONE, }; /**