From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org, Nikita Pettik <korablev@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v2 2/3] Introduce "none" and "binary" collations Date: Tue, 13 Nov 2018 15:02:39 +0300 [thread overview] Message-ID: <e1e6f1ad-a2bc-58f3-f6ce-6cbacd14dce0@tarantool.org> (raw) In-Reply-To: <8e9bca5d0f620a5cbd3bdffe521cecdfd1ad0ffd.1542066357.git.korablev@tarantool.org> 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 <v.shpilevoy@tarantool.org> 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,
next prev parent reply other threads:[~2018-11-13 12:02 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 ` Vladislav Shpilevoy [this message] 2018-11-13 22:37 ` [tarantool-patches] " n.pettik 2018-11-14 11:52 ` Vladislav Shpilevoy 2018-11-14 20:59 ` n.pettik 2018-11-15 11:04 ` Vladislav Shpilevoy 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=e1e6f1ad-a2bc-58f3-f6ce-6cbacd14dce0@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