On 31 Oct 2018, at 15:34, Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
wrote:
Hi! Thanks for the patch! See 3 comments below.
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 20e79f9fe..ecdc199d9 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -78,6 +78,23 @@ extern const struct key_part_def key_part_def_default;
*/
#define COLL_NONE UINT32_MAX
+/**
+ * In SQL explicitly specified binary collation and absence of
+ * any collation are different in behaviour: according to ANSI
+ * it is prohibited to compare strings with different explicitly
+ * indicated collations. However, if one of collation is default,
+ * (i.e. absent) the second one will be forced.
+ * So, lets introduce another id to indicate explicitly specified
+ * binary collation.
1. Sorry, I am not sure that we can use imperative while
describing not functions.
2. I see, that you actually created a 'phantom' collation.
A collation, that has no a record in the collation cache,
but is visible to a user via space format. I think for
externally visible changes you should consult Kostja.
Alternatively, it is possible to create binary collation
in the same way as unicode and unicode_ci - via insertion
into _collation in upgrade script.
Also, I see a bug that we can create a collation in
_collation with id = COLL_NONE and COLL_BINARY, but which
actually are not NONE nor BINARY. Storing such identifiers
in _collation should be prohibited (if we will leave current
'phantom' binary collation as is). Furthermore COLL_NONE
for unknown reason is declared in key_def.h instead of
coll_id.h. It should be moved out. It is worth to create a
separate commit with refactoring right before this one.
+ */
+#define COLL_BINARY (UINT32_MAX - 1)
+
+static inline bool
+coll_is_missing(uint32_t coll_id)
+{
+ return coll_id == COLL_NONE || coll_id == COLL_BINARY;
+}
Hence, this should be moved to coll_id.h as well and
renamed to coll_id_is_missing.
diff --git a/test/sql/collation.test.lua b/test/sql/collation.test.lua
index 935dea824..f9d653717 100644
--- a/test/sql/collation.test.lua
+++ b/test/sql/collation.test.lua
@@ -42,4 +42,12 @@ cn = remote.connect(box.cfg.listen)
cn:execute('select 1 limit ? collate not_exist', {1})
cn:close()
+
+-- Explicitly set BINARY collation has ID.
3. Please, add more tests, especially for box when you set id to
4294967294.
+--
+box.sql.execute("CREATE TABLE t (id INT PRIMARY KEY, a TEXT, b TEXT COLLATE
BINARY);")
+box.space.T:format()[2]['collation']
+box.space.T:format()[3]['collation']
+box.sql.execute("DROP TABLE t;")
+
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
