[tarantool-patches] Re: [PATCH v2 2/3] Introduce "none" and "binary" collations
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Nov 13 15:02:39 MSK 2018
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 at 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,
More information about the Tarantool-patches
mailing list