<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">See below 6 comments.<br class=""><div><br class=""><blockquote type="cite" class=""><div class="">28 февр. 2018 г., в 19:10, Nikita Pettik <<a href="mailto:korablev@tarantool.org" class="">korablev@tarantool.org</a>> написал(а):</div><br class="Apple-interchange-newline"><div class=""><div class="">Originally, collation can be assigned only to index parts. However, SQL<br class="">standard says that each column is capable of having such property. Thus,<br class="">in order to support standard, add collation id to field_def and pointer<br class="">to collation to tuple_field.<br class=""><br class="">Closes #2937<br class="">---<br class=""> src/box/<a href="http://alter.cc" class="">alter.cc</a> | 7 +++++++<br class=""> src/box/field_def.c | 5 ++++-<br class=""> src/box/field_def.h | 2 ++<br class=""> src/box/lua/schema.lua | 7 +++++++<br class=""> src/box/sql.c | 11 ++++++++++-<br class=""> src/box/tuple_format.c | 14 +++++++++++++-<br class=""> src/box/tuple_format.h | 3 +++<br class=""> test/engine/iterator.result | 2 +-<br class=""> 8 files changed, 47 insertions(+), 4 deletions(-)<br class=""><br class="">diff --git a/src/box/<a href="http://alter.cc" class="">alter.cc</a> b/src/box/<a href="http://alter.cc" class="">alter.cc</a><br class="">index 1fcb2b5d3..30baacbb9 100644<br class="">--- a/src/box/<a href="http://alter.cc" class="">alter.cc</a><br class="">+++ b/src/box/<a href="http://alter.cc" class="">alter.cc</a><br class="">@@ -374,6 +374,13 @@ field_def_decode(struct field_def *field, const char **data,<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> "nullable action properties", fieldno +<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> TUPLE_INDEX_BASE));<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>}<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>if (field->coll_id != COLL_NONE &&<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span> field->type != FIELD_TYPE_STRING &&<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span> field->type != FIELD_TYPE_SCALAR) {<br class=""></div></div></blockquote><div><br class=""></div><div>1. We have also FIELD_TYPE_ANY, which can not be indexed and because of it this type is not checked in <a href="http://key_def.cc" class="">key_def.cc</a> in key_def_decode_parts. But for non-indexed field it can appear and can have a collection, I think.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>tnt_raise(ClientError, errcode, tt_cstr(space_name, name_len),<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> tt_sprintf("collation is reasonable only for "<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> "string and scalar parts"));<br class=""></div></div></blockquote><div><br class=""></div><div>2. It is not part. Part is an index term. For regular fields, please, use term 'field'.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>}<br class=""> }<br class=""><br class=""> /**<br class="">diff --git a/src/box/field_def.c b/src/box/field_def.c<br class="">index 166baa7c5..e48896569 100644<br class="">--- a/src/box/field_def.c<br class="">+++ b/src/box/field_def.c<br class="">@@ -31,6 +31,7 @@<br class=""><br class=""> #include "field_def.h"<br class=""> #include "trivia/util.h"<br class="">+#include "key_def.h"<br class=""><br class=""> const char *field_type_strs[] = {<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>/* [FIELD_TYPE_ANY] = */ "any",<br class="">@@ -92,6 +93,7 @@ const struct opt_def field_def_reg[] = {<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>OPT_DEF("is_nullable", OPT_BOOL, struct field_def, is_nullable),<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>OPT_DEF_ENUM("nullable_action", on_conflict_action, struct field_def,<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> nullable_action, NULL),<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>OPT_DEF("collation", OPT_UINT32, struct field_def, coll_id),<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>OPT_END,<br class=""> };<br class=""><br class="">@@ -99,7 +101,8 @@ const struct field_def field_def_default = {<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>.type = FIELD_TYPE_ANY,<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>.name = NULL,<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>.is_nullable = false,<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>.nullable_action = ON_CONFLICT_ACTION_DEFAULT<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>.nullable_action = ON_CONFLICT_ACTION_DEFAULT,<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>.coll_id = COLL_NONE<br class=""> };<br class=""><br class=""> enum field_type<br class="">diff --git a/src/box/field_def.h b/src/box/field_def.h<br class="">index 92bad6b9f..61f3e49db 100644<br class="">--- a/src/box/field_def.h<br class="">+++ b/src/box/field_def.h<br class="">@@ -108,6 +108,8 @@ struct field_def {<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>bool is_nullable;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>/** Action to perform if NULL constraint failed. */<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>enum on_conflict_action nullable_action;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>/** Collation ID for string comparison. */<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>uint32_t coll_id;<br class=""> };<br class=""><br class=""> #if defined(__cplusplus)<br class="">diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua<br class="">index 49b832c77..204e7e85f 100644<br class="">--- a/src/box/lua/schema.lua<br class="">+++ b/src/box/lua/schema.lua<br class="">@@ -338,6 +338,13 @@ function update_format(format)<br class=""> field.name = v;<br class=""> elseif k == 2 then<br class=""> field.type = v;<br class="">+ elseif k == 'collation' then<br class="">+ local coll = box.space._collation.index.name:get{v}<br class="">+ if not coll then<br class="">+ box.error(box.error.ILLEGAL_PARAMS,<br class="">+ "format[" .. i .. "]: collation was not found by name '" .. v .. "'")<br class="">+ end<br class="">+ field[k] = coll[1]<br class=""></div></div></blockquote><div><br class=""></div><div>3. Please, rebase on the latest 2.0. Since you patch was created, this code is slightly changed.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""> else<br class=""> field[k] = v<br class=""> end<br class="">diff --git a/src/box/sql.c b/src/box/sql.c<br class="">index 9ce270da9..b5762a297 100644<br class="">--- a/src/box/sql.c<br class="">+++ b/src/box/sql.c<br class="">@@ -1561,7 +1561,12 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)<br class=""><br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>for (i = 0; i < n; i++) {<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>const char *t;<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>p = enc->encode_map(p, 4);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>struct coll *coll = NULL;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>if (aCol[i].zColl &&<br class=""></div></div></blockquote><div><br class=""></div><div>4. I prefer explicit != NULL, but it is up to you. AFAIK pointers checking on NULL is not standardised in Tarantool((</div><br class=""><blockquote type="cite" class=""><div class=""><div class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> sqlite3StrICmp(aCol[i].zColl, "binary") != 0) {<br class=""></div></div></blockquote><div><br class=""></div><span class="">5. It is not remark, just a subject to discuss. Do we want continue using sqlite string functions? For example, sqlite3StrICmp can already be replaced with strcasecmp.<br class=""></span><span class=""><br class=""></span><blockquote type="cite" class=""><div class=""><div class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>coll = sqlite3FindCollSeq(NULL, aCol[i].zColl, 0);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>}<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>p = enc->encode_map(p, coll ? 5 : 4);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>p = enc->encode_str(p, "name", 4);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>p = enc->encode_str(p, aCol[i].zName, strlen(aCol[i].zName));<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>p = enc->encode_str(p, "type", 4);<br class="">@@ -1579,6 +1584,10 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>assert(aCol[i].notNull < on_conflict_action_MAX);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>const char *action = on_conflict_action_strs[aCol[i].notNull];<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>p = enc->encode_str(p, action, strlen(action));<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>if (coll != NULL) {</div></div></blockquote><blockquote type="cite" class=""><div class=""><div class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>p = enc->encode_str(p, "collation", strlen("collation"));<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>p = enc->encode_uint(p, coll->id);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>}<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>}<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>return (int)(p - base);<br class=""> }<br class="">diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c<br class="">index e05998bf9..17f579710 100644<br class="">--- a/src/box/tuple_format.c<br class="">+++ b/src/box/tuple_format.c<br class="">@@ -37,7 +37,8 @@ static intptr_t recycled_format_ids = FORMAT_ID_NIL;<br class=""> static uint32_t formats_size = 0, formats_capacity = 0;<br class=""><br class=""> static const struct tuple_field tuple_field_default = {<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false, ON_CONFLICT_ACTION_DEFAULT<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false,<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>ON_CONFLICT_ACTION_DEFAULT, NULL<br class=""> };<br class=""><br class=""> /**<br class="">@@ -59,6 +60,17 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>format->fields[i].type = fields[i].type;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>format->fields[i].offset_slot = TUPLE_OFFSET_SLOT_NIL;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>format->fields[i].nullable_action = fields[i].nullable_action;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>struct coll *coll = NULL;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>uint32_t coll_id = fields[i].coll_id;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>if (coll_id != COLL_NONE) {<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>coll = coll_by_id(coll_id);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>if (coll == NULL) {<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>diag_set(ClientError,ER_WRONG_COLLATION_OPTIONS,<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> i + 1, "collation was not found by ID");<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>return -1;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>}<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>}<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>format->fields[i].coll = coll;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>if (i + 1 > format->min_field_count && !fields[i].is_nullable)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>format->min_field_count = i + 1;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>}<br class="">diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h<br class="">index c4074f014..70565aef9 100644<br class="">--- a/src/box/tuple_format.h<br class="">+++ b/src/box/tuple_format.h<br class="">@@ -35,6 +35,7 @@<br class=""> #include "field_def.h"<br class=""> #include "errinj.h"<br class=""> #include "tuple_dictionary.h"<br class="">+#include "coll_cache.h"<br class=""><br class=""> #if defined(__cplusplus)<br class=""> extern "C" {<br class="">@@ -98,6 +99,8 @@ struct tuple_field {<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>bool is_key_part;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>/** Action to perform if NULL constraint failed. */<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>enum on_conflict_action nullable_action;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>/** Collation definition for string comparison */<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>struct coll *coll;<br class=""> };<br class=""><br class=""> /**<br class="">diff --git a/test/engine/iterator.result b/test/engine/iterator.result<br class="">index e19886677..e54e21203 100644<br class="">--- a/test/engine/iterator.result<br class="">+++ b/test/engine/iterator.result<br class=""></div></div></blockquote><div><br class=""></div><div>6. Please, add some tests, that check you can specify collation for a tuple field in lua.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="">@@ -4215,7 +4215,7 @@ s:replace{35}<br class=""> ...<br class=""> state, value = gen(param,state)<br class=""> ---<br class="">-- error: 'builtin/box/schema.lua:981: usage: next(param, state)'<br class="">+- error: 'builtin/box/schema.lua:988: usage: next(param, state)'<br class=""> ...<br class=""> value<br class=""> ---<br class="">-- <br class="">2.15.1<br class=""><br class=""></div></div></blockquote></div><br class=""></body></html>