From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: "n.pettik" <korablev@tarantool.org>, tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH v2 2/3] Introduce "none" and "binary" collations Date: Thu, 15 Nov 2018 14:04:05 +0300 [thread overview] Message-ID: <ba4a3098-c179-5dab-1322-59bfb52b189a@tarantool.org> (raw) In-Reply-To: <9DBC0637-55A0-47E7-815F-CEA5FA1189AA@tarantool.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 != <strange_constant>'. > > 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, }; /**
next prev parent reply other threads:[~2018-11-15 11:04 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-11-13 0:07 [tarantool-patches] [PATCH v2 0/3] Change collation compatibility rules according to ANSI SQL Nikita Pettik 2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 1/3] sql: do not add explicit <COLLATE "BINARY"> clause Nikita Pettik 2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 2/3] Introduce "none" and "binary" collations Nikita Pettik 2018-11-13 12:02 ` [tarantool-patches] " Vladislav Shpilevoy 2018-11-13 22:37 ` n.pettik 2018-11-14 11:52 ` Vladislav Shpilevoy 2018-11-14 20:59 ` n.pettik 2018-11-15 11:04 ` Vladislav Shpilevoy [this message] 2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 3/3] sql: change collation compatibility rules Nikita Pettik 2018-11-13 12:02 ` [tarantool-patches] " Vladislav Shpilevoy 2018-11-13 22:37 ` n.pettik 2018-11-15 11:24 ` [tarantool-patches] Re: [PATCH v2 0/3] Change collation compatibility rules according to ANSI SQL Kirill Yukhin
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=ba4a3098-c179-5dab-1322-59bfb52b189a@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v2 2/3] Introduce "none" and "binary" collations' \ /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