[tarantool-patches] Re: [PATCH v2 2/3] Introduce "none" and "binary" collations

[Vladislav Shpilevoy]

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,
  };
  
  /**