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 AA6CA2EE0D for ; Tue, 13 Nov 2018 07:02:43 -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 xf_oG3DFE5hA for ; Tue, 13 Nov 2018 07:02:43 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 3EF3B2EE0C for ; Tue, 13 Nov 2018 07:02:43 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 2/3] Introduce "none" and "binary" collations References: <8e9bca5d0f620a5cbd3bdffe521cecdfd1ad0ffd.1542066357.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Tue, 13 Nov 2018 15:02:39 +0300 MIME-Version: 1.0 In-Reply-To: <8e9bca5d0f620a5cbd3bdffe521cecdfd1ad0ffd.1542066357.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: tarantool-patches@freelists.org, Nikita Pettik Hi! Thanks for the fixes! Please, remember to put branch/issue link into a covering letter next time. See 5 comments below, my fixes at the end of the email and on the branch. > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 6d2c59bbc..2eb9f53e8 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -403,7 +403,7 @@ field_def_decode(struct field_def *field, const char **data, > "nullable action properties", fieldno + > TUPLE_INDEX_BASE)); > } > - if (field->coll_id != COLL_NONE && > + if (field->coll_id != 0 && 1. Why not to leave enum COLL_NONE, but change its value to 0? I guess, it is a purpose of define/enum features - define a name for a constant to be able to change the value under the hood. It could have reduced the diff. > field->type != FIELD_TYPE_STRING && > field->type != FIELD_TYPE_SCALAR && > field->type != FIELD_TYPE_ANY) { > diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c > index 3cf3a835d..352745e0e 100644 > --- a/src/box/sql/callback.c > +++ b/src/box/sql/callback.c > @@ -42,13 +42,22 @@ > struct coll * > sql_get_coll_seq(Parse *parser, const char *name, uint32_t *coll_id) > { > - if (name == NULL || strcasecmp(name, "binary") == 0) { > - *coll_id = COLL_NONE; > - return NULL; > + if (name == NULL) { > + *coll_id = 0; > + return coll_by_id(0)->coll; > } > - struct coll_id *p = coll_by_name(name, strlen(name)); > + struct coll_id *p; > + /* > + * In SQL all identifiers should be uppercased, so > + * to avoid mess lets simple search binary (since it is > + * sort of "special" collation) ignoring case at all. > + */ 2. I am not sure if it should be so special. I think, we should treat it just like any other collation. If tests have BINARY uppercased, then they should be fixed, I guess. > + if (strcasecmp(name, "binary") == 0) > + p = coll_by_name("binary", strlen("binary")); > + else > + p = coll_by_name(name, strlen(name)); > if (p == NULL) { > - *coll_id = COLL_NONE; > + *coll_id = 0; > sqlite3ErrorMsg(parser, "no such collation sequence: %s", > name); > return NULL; > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index 8c34cbb3d..580cf1e60 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -506,7 +506,7 @@ case_type##ICUFunc(sqlite3_context *context, int argc, sqlite3_value **argv) \ > UErrorCode status = U_ZERO_ERROR; \ > struct coll *coll = sqlite3GetFuncCollSeq(context); \ > const char *locale = NULL; \ > - if (coll != NULL) { \ > + if (coll != NULL && coll->collator != NULL) { \ 3. Why do you use coll->collator != 0 instead of coll->type == COLL_TYPE_ICU? I do not think it is safe since in future we can move collator into a union, which will not be NULL for a non-ICU collation. > locale = ucol_getLocaleByType(coll->collator, \ > ULOC_VALID_LOCALE, &status); \ > } > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c > index 5bdf102e7..8e5aea51b 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, COLL_NONE, > + ON_CONFLICT_ACTION_DEFAULT, NULL, 0, > }; 4. Are you sure that changing on_conflict_action belongs to this patch? 5. How about tests like this one? box.schema.create_space('test', {format = {{1, 'unsigned'}, {2, 'string', collation = 'binary'}}}) I mean specifying collations via Lua. And what happens if I use collation = 'none' in a space format or index parts? I think, it should throw an error, but it is up to Kostja or Kirill. ================================================== commit 58691e9f2f593771865264c6aaed93b36cc13c08 Author: Vladislav Shpilevoy Date: Tue Nov 13 14:56:17 2018 +0300 Speculative review fixes diff --git a/src/box/alter.cc b/src/box/alter.cc index 2eb9f53e8..474ef791e 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -403,7 +403,7 @@ field_def_decode(struct field_def *field, const char **data, "nullable action properties", fieldno + TUPLE_INDEX_BASE)); } - if (field->coll_id != 0 && + if (field->coll_id != COLL_NONE && field->type != FIELD_TYPE_STRING && field->type != FIELD_TYPE_SCALAR && field->type != FIELD_TYPE_ANY) { @@ -2676,7 +2676,7 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event) * under the hood. Hence, we can rely on the * fact that "none" collation features id == 0. */ - if (old_id == 0) { + if (old_id == COLL_NONE) { tnt_raise(ClientError, ER_DROP_COLLATION, "none", "system collation"); } diff --git a/src/box/coll_id.h b/src/box/coll_id.h index 1b67a3f86..82e50d11b 100644 --- a/src/box/coll_id.h +++ b/src/box/coll_id.h @@ -37,6 +37,10 @@ extern "C" { #endif /* defined(__cplusplus) */ +enum { + COLL_NONE = 0, +}; + struct coll_id_def; struct coll; diff --git a/src/box/field_def.c b/src/box/field_def.c index 3e63e12a3..3a9ff3703 100644 --- a/src/box/field_def.c +++ b/src/box/field_def.c @@ -123,7 +123,7 @@ const struct field_def field_def_default = { .name = NULL, .is_nullable = false, .nullable_action = ON_CONFLICT_ACTION_DEFAULT, - .coll_id = 0, + .coll_id = COLL_NONE, .default_value = NULL, .default_value_expr = NULL }; diff --git a/src/box/key_def.c b/src/box/key_def.c index 6802489f1..3a560bb06 100644 --- a/src/box/key_def.c +++ b/src/box/key_def.c @@ -41,7 +41,7 @@ const char *sort_order_strs[] = { "asc", "desc", "undef" }; const struct key_part_def key_part_def_default = { 0, field_type_MAX, - 0, + COLL_NONE, false, ON_CONFLICT_ACTION_DEFAULT, SORT_ORDER_ASC @@ -174,7 +174,7 @@ key_def_new(const struct key_part_def *parts, uint32_t part_count) for (uint32_t i = 0; i < part_count; i++) { const struct key_part_def *part = &parts[i]; struct coll *coll = NULL; - if (part->coll_id != 0) { + if (part->coll_id != COLL_NONE) { struct coll_id *coll_id = coll_by_id(part->coll_id); if (coll_id == NULL) { diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, @@ -223,7 +223,7 @@ box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count) key_def_set_part(key_def, item, fields[item], (enum field_type)types[item], ON_CONFLICT_ACTION_DEFAULT, - NULL, 0, SORT_ORDER_ASC); + NULL, COLL_NONE, SORT_ORDER_ASC); } key_def_set_cmp(key_def); return key_def; @@ -319,7 +319,7 @@ key_def_sizeof_parts(const struct key_part_def *parts, uint32_t part_count) for (uint32_t i = 0; i < part_count; i++) { const struct key_part_def *part = &parts[i]; int count = 2; - if (part->coll_id != 0) + if (part->coll_id != COLL_NONE) count++; if (part->is_nullable) count++; @@ -329,7 +329,7 @@ key_def_sizeof_parts(const struct key_part_def *parts, uint32_t part_count) assert(part->type < field_type_MAX); size += mp_sizeof_str(strlen(PART_OPT_TYPE)); size += mp_sizeof_str(strlen(field_type_strs[part->type])); - if (part->coll_id != 0) { + if (part->coll_id != COLL_NONE) { size += mp_sizeof_str(strlen(PART_OPT_COLLATION)); size += mp_sizeof_uint(part->coll_id); } @@ -348,7 +348,7 @@ key_def_encode_parts(char *data, const struct key_part_def *parts, for (uint32_t i = 0; i < part_count; i++) { const struct key_part_def *part = &parts[i]; int count = 2; - if (part->coll_id != 0) + if (part->coll_id != COLL_NONE) count++; if (part->is_nullable) count++; @@ -361,7 +361,7 @@ key_def_encode_parts(char *data, const struct key_part_def *parts, assert(part->type < field_type_MAX); const char *type_str = field_type_strs[part->type]; data = mp_encode_str(data, type_str, strlen(type_str)); - if (part->coll_id != 0) { + if (part->coll_id != COLL_NONE) { data = mp_encode_str(data, PART_OPT_COLLATION, strlen(PART_OPT_COLLATION)); data = mp_encode_uint(data, part->coll_id); @@ -431,7 +431,7 @@ key_def_decode_parts_166(struct key_part_def *parts, uint32_t part_count, part->is_nullable = (part->fieldno < field_count ? fields[part->fieldno].is_nullable : key_part_def_default.is_nullable); - part->coll_id = 0; + part->coll_id = COLL_NONE; } return 0; } @@ -488,7 +488,7 @@ key_def_decode_parts(struct key_part_def *parts, uint32_t part_count, "index part: unknown field type"); return -1; } - if (part->coll_id != 0 && + if (part->coll_id != COLL_NONE && part->type != FIELD_TYPE_STRING && part->type != FIELD_TYPE_SCALAR) { diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc index e5e09b042..7cae436f1 100644 --- a/src/box/lua/space.cc +++ b/src/box/lua/space.cc @@ -299,7 +299,7 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i) lua_pushboolean(L, key_part_is_nullable(part)); lua_setfield(L, -2, "is_nullable"); - if (part->coll_id != 0) { + if (part->coll_id != COLL_NONE) { struct coll_id *coll_id = coll_by_id(part->coll_id); assert(coll_id != NULL); diff --git a/src/box/sql.c b/src/box/sql.c index 686d32335..caa66144f 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -379,7 +379,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) if (def != NULL && i < def->part_count) part->coll_id = def->parts[i].coll_id; else - part->coll_id = 0; + part->coll_id = COLL_NONE; } struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts, field_count); @@ -1139,7 +1139,7 @@ sql_encode_table(struct region *region, struct Table *table, uint32_t *size) struct field_def *field = &def->fields[i]; const char *default_str = field->default_value; int base_len = 5; - if (cid != 0) + if (cid != COLL_NONE) base_len += 1; if (default_str != NULL) base_len += 1; @@ -1160,7 +1160,7 @@ sql_encode_table(struct region *region, struct Table *table, uint32_t *size) const char *action = on_conflict_action_strs[def->fields[i].nullable_action]; mpstream_encode_str(&stream, action); - if (cid != 0) { + if (cid != COLL_NONE) { mpstream_encode_str(&stream, "collation"); mpstream_encode_uint(&stream, cid); } @@ -1285,13 +1285,13 @@ sql_encode_index_parts(struct region *region, const struct field_def *fields, action_is_nullable(fields[col].nullable_action)); /* Do not decode default collation. */ uint32_t cid = part->coll_id; - mpstream_encode_map(&stream, 5 + (cid != 0)); + mpstream_encode_map(&stream, 5 + (cid != COLL_NONE)); mpstream_encode_str(&stream, "type"); mpstream_encode_str(&stream, field_type_strs[fields[col].type]); mpstream_encode_str(&stream, "field"); mpstream_encode_uint(&stream, col); - if (cid != 0) { + if (cid != COLL_NONE) { mpstream_encode_str(&stream, "collation"); mpstream_encode_uint(&stream, cid); } diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 929d20dbf..5b3348bd2 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2399,7 +2399,7 @@ index_fill_def(struct Parse *parse, struct index *index, uint32_t coll_id; if (expr->op == TK_COLLATE) { sql_get_coll_seq(parse, expr->u.zToken, &coll_id); - if (coll_id == 0 && + if (coll_id == COLL_NONE && strcasecmp(expr->u.zToken, "binary") != 0) { diag_set(ClientError, ER_NO_SUCH_COLLATION, expr->u.zToken); diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c index 352745e0e..927eb2643 100644 --- a/src/box/sql/callback.c +++ b/src/box/sql/callback.c @@ -43,8 +43,8 @@ struct coll * sql_get_coll_seq(Parse *parser, const char *name, uint32_t *coll_id) { if (name == NULL) { - *coll_id = 0; - return coll_by_id(0)->coll; + *coll_id = COLL_NONE; + return coll_by_id(COLL_NONE)->coll; } struct coll_id *p; /* @@ -57,7 +57,7 @@ sql_get_coll_seq(Parse *parser, const char *name, uint32_t *coll_id) else p = coll_by_name(name, strlen(name)); if (p == NULL) { - *coll_id = 0; + *coll_id = COLL_NONE; sqlite3ErrorMsg(parser, "no such collation sequence: %s", name); return NULL; diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index e52cd6407..4d1c1a634 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -194,7 +194,7 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_found, uint32_t *coll_id) { struct coll *coll = NULL; *is_found = false; - *coll_id = 0; + *coll_id = COLL_NONE; while (p != NULL) { int op = p->op; if (p->flags & EP_Generic) diff --git a/src/box/sql/select.c b/src/box/sql/select.c index cea453f08..dfa6ed8e0 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -1345,7 +1345,7 @@ sql_key_info_new(sqlite3 *db, uint32_t part_count) struct key_part_def *part = &key_info->parts[i]; part->fieldno = i; part->type = FIELD_TYPE_SCALAR; - part->coll_id = 0; + part->coll_id = COLL_NONE; part->is_nullable = false; part->nullable_action = ON_CONFLICT_ACTION_ABORT; part->sort_order = SORT_ORDER_ASC; @@ -1961,7 +1961,7 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse, /* Parsing contexts */ pTab->def->fields[i].type = sql_affinity_to_field_type(affinity); bool is_found; uint32_t coll_id; - if (pTab->def->fields[i].coll_id == 0 && + if (pTab->def->fields[i].coll_id == COLL_NONE && sql_expr_coll(pParse, p, &is_found, &coll_id) && is_found) pTab->def->fields[i].coll_id = coll_id; } @@ -2160,7 +2160,7 @@ multi_select_coll_seq_r(Parse *parser, Select *p, int n, bool *is_found, coll_id); } else { coll = NULL; - *coll_id = 0; + *coll_id = COLL_NONE; } assert(n >= 0); /* iCol must be less than p->pEList->nExpr. Otherwise an error would @@ -2233,7 +2233,7 @@ sql_multiselect_orderby_to_key_info(struct Parse *parse, struct Select *s, } else { multi_select_coll_seq(parse, s, item->u.x.iOrderByCol - 1, &id); - if (id != 0) { + if (id != COLL_NONE) { const char *name = coll_by_id(id)->name; order_by->a[i].pExpr = sqlite3ExprAddCollateString(parse, term, diff --git a/src/box/sql/where.c b/src/box/sql/where.c index 1db4db874..8c78c0c9b 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -2806,7 +2806,7 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder, /* WHERE clause information */ part.nullable_action = ON_CONFLICT_ACTION_ABORT; part.is_nullable = false; part.sort_order = SORT_ORDER_ASC; - part.coll_id = 0; + part.coll_id = COLL_NONE; struct key_def *key_def = key_def_new(&part, 1); if (key_def == NULL) { diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index 8e5aea51b..4a8905262 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_DEFAULT, NULL, 0, + ON_CONFLICT_ACTION_DEFAULT, NULL, COLL_NONE, }; /** @@ -67,7 +67,7 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys, format->fields[i].nullable_action = fields[i].nullable_action; struct coll *coll = NULL; uint32_t cid = fields[i].coll_id; - if (cid != 0) { + if (cid != COLL_NONE) { struct coll_id *coll_id = coll_by_id(cid); if (coll_id == NULL) { diag_set(ClientError,ER_WRONG_COLLATION_OPTIONS,