<HTML><BODY><br><br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Вторник, 22 мая 2018, 22:10 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:<br>
<br>
<div id="">
<div class="js-helper js-readmsg-msg">
<style type="text/css"></style>
<div>
<base target="_self" href="https://e.mail.ru/">
<div id="style_15270162280000000791_BODY">Hello. Thanks for the patch! See 26 comments below.<br>
<br>
On 22/05/2018 19:03, Ivan Koptelov wrote:<br>
> Currently there is a problem with<br>
> non-deterministic nullability in<br>
> index_def. To see the error just<br>
> uncomment printfs on build.c 2999<br>
> and run any sql-tap test that<br>
> has to do with indexes, e.g<br>
> distinct.test.lua.<br>
> github branch: <a href="https://github.com/tarantool/tarantool/tree/sb/gh-3369-use-index-def-in-select-and-where" target="_blank">https://github.com/tarantool/tarantool/tree/sb/gh-3369-use-index-def-in-select-and-where</a><br>
> ---<br>
> src/box/sql/build.c | 179 +++++++++++++++++++++++++++++++++++++++++++++++-<br>
> src/box/sql/sqliteInt.h | 2 +<br>
> 2 files changed, 179 insertions(+), 2 deletions(-)<br>
> <br>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c<br>
> index 742f8a572..6bd70fa9f 100644<br>
> --- a/src/box/sql/build.c<br>
> +++ b/src/box/sql/build.c<br>
> @@ -43,6 +43,7 @@<br>
> * COMMIT<br>
> * ROLLBACK<br>
> */<br>
> +#include <ctype.h><br>
<br>
1. See the comment 2 and remove it, please.<br>
<br>
> #include "sqliteInt.h"<br>
> #include "vdbeInt.h"<br>
> #include "tarantoolInt.h"<br>
> @@ -2841,9 +2842,173 @@ index_is_unique(Index *idx)<br>
> return tnt_index->def->opts.is_unique;<br>
> }<br>
> <br>
> +/*<br>
> + * In CREATE INDEX requests user may set binary collation with COLLATE<br>
> + * [BINARY, binary, "binary" and "bInarY"] This function is to handle all<br>
> + * these cases.<br>
> + */<br>
> +bool is_binary_collation(const char *coll_name)<br>
> +{<br>
> + char *upper_case_name =<br>
> + malloc(sizeof(*coll_name) * strlen(coll_name));<br>
> + for (int i = 0; i < (int) strlen(coll_name); i++) {<br>
> + upper_case_name[i] = toupper(coll_name[i]);<br>
> + }<br>
> +<br>
> + bool res = (strcmp(upper_case_name, "BINARY") == 0);<br>
> + free(upper_case_name);<br>
> + return res;<br>
<br>
2. All this function is identical to simple strcasecmp(coll_name, "binary").<br>
<br>
> +}<br>
> +<br>
> +//bool is_index_equal(Index * old, struct index_def *new)<br>
> +//{<br>
> +// assert(strcmp(old->zName, new->name);<br>
> +// return true;<br>
> +//}<br>
<br>
3. Garbage diff.<br>
<br>
> +<br>
> +struct index_def *create_index_def(Table *pTab, const char *name,<br>
> + uint32_t name_len, int onError,<br>
> + ExprList *pList, u8 idxType)<br>
<br>
4. According to Tarantool code style you should wrap line after return type<br>
definition. And please, use snake_case for variable names.<br>
<br>
> +{<br>
> + struct space_def *space_def = pTab->def;<br>
> + struct space *space = space_by_id(space_def->id);<br>
<br>
5. space_by_id takes a space from the space cache, but during<br>
'CREATE TABLE' generation the space is not in the cache. This will<br>
return NULL.<br>
<br>
You must not use space * here at all. I see, that you use it<br>
in a single place to find a primary index key_def, but you can get it<br>
from the pTab from the first pIndex - it is always primary one.<br>
<br>
> +<br>
> + uint32_t iid = 0;<br>
> + Index *temp = pTab->pIndex;<br>
> + while (temp != NULL) {<br>
> + iid++;<br>
> + temp = temp->pNext;<br>
<br>
6. Bad idea. You must take Index * as the function argument, and initialize its<br>
index_def.<br>
<br>
> + }<br>
> +<br>
> + struct index_opts *opts = (struct index_opts *) malloc(sizeof(*opts));<br>
<br>
7. Why can not you declare it on the stack?<br>
<br>
> + index_opts_create(opts);<br>
> + opts->sql = NULL;<br>
> +<br>
> + if (onError == ON_CONFLICT_ACTION_NONE) {<br>
> + opts->is_unique = false;<br>
> + } else {<br>
> + opts->is_unique = true;<br>
> + }<br>
<br>
8. Do not put 'if' body in {} when it consists of a single line.<br>
<br>
9. Here you can simply do this:<br>
opts->is_unique = onError != ON_CONFLICT_ACTION_NONE;<br>
<br>
> +<br>
> + struct key_part_def *part_def_tmp = (struct key_part_def *)<br>
> + malloc(sizeof(*part_def_tmp) * pList->nExpr);<br>
<br>
10. Please, do not do this. Use key_def_new + multiple key_def_set_part.<br>
You use too many mallocs.<br>
<br>
> +<br>
> + uint32_t part_count = 0;<br>
> + int i = 0;<br>
> + struct ExprList_item *pListItem;<br>
> +<br>
> + /*<br>
> + * In case of ordinary index creation we want ot store a<br>
> + * SQL statement of this creation.<br>
> + */<br>
> + if (idxType == SQLITE_IDXTYPE_APPDEF) {<br>
> + asprintf(&opts->sql,"CREATE INDEX %s ON %s(",<br>
> + name, space_def->name);<br>
<br>
11. You should check if asprintf is failed. Or do not use asprintf at<br>
all. See below how.<br>
<br>
> + }<br>
> +<br>
> + for (i = 0, pListItem = pList->a; i < pList->nExpr; pListItem++, i++) {<br>
> + Expr *pCExpr = sqlite3ExprSkipCollate(pListItem->pExpr);<br>
> +<br>
> + if (pCExpr->op == TK_COLUMN) {<br>
> + uint32_t fieldno = pCExpr->iColumn;<br>
> + part_def_tmp[i].fieldno = fieldno;<br>
> + part_def_tmp[i].nullable_action =<br>
> + ON_CONFLICT_ACTION_DEFAULT;<br>
<br>
12. Wrong - action default is internal thing, that can not be<br>
set in key_part actually. You must set action to the same value as<br>
the space field is set.<br>
<br>
> + part_def_tmp[i].type =<br>
> + space_def->fields[fieldno].type;<br>
> +<br>
> +// part_def_tmp[i].is_nullable =<br>
> +// (fieldno < space_def->field_count ?<br>
> +// space_def->fields[fieldno].is_nullable :<br>
> +// false);<br>
> +<br>
> + part_def_tmp[i].is_nullable =<br>
> + table_column_is_nullable(pTab->def, fieldno);<br>
<br>
13. Please, do not use table_column_is_nullable. It does space_by_id(def->id),<br>
but here def->id is not defined when you create a primary index inside a CREATE<br>
TABLE request.<br>
<br>
> +<br>
> + part_def_tmp[i].coll_id =<br>
> + space_def->fields[fieldno].coll_id ?<br>
> + space_def->fields[fieldno].coll_id : COLL_NONE;<br>
<br>
14. Do not use integers as booleans.<br>
<br>
15. Why not just part_def_tmp[i].coll_id = space_def->fields[fieldno].coll_id?<br>
Besides, coll_id == 0 is valid collation identifier. It is not the same as COLL_NONE.<br>
<br>
> + part_count++;<br>
<br>
16. Why do you need this counter? You already know part count - it is equal<br>
to pList->nExpr.<br>
<br>
> +<br>
> + if (opts->sql != NULL) {<br>
<br>
17. How can it be NULL?<br>
<br>
> + if (part_count == 1) {<br>
> + asprintf(&opts->sql,"%s%s,",opts->sql,<br>
> + space_def->fields[fieldno].name);<br>
> + } else {<br>
> + asprintf(&opts->sql,"%s %s,",opts->sql,<br>
> + space_def->fields[fieldno].name);<br>
> + }<br>
> + }<br>
> + } else {<br>
> + assert(pCExpr->op == TK_COLUMN);<br>
<br>
18. So I see this:<br>
if (something) {<br>
...<br>
} else {<br>
assert(something);<br>
}<br>
<br>
Very strange. This assert fails always.<br>
<br>
> + }<br>
> +<br>
> + if (pListItem->pExpr->op == TK_COLLATE) {<br>
<br>
19. Here you completely skipped column initialization when it has<br>
COLLATE. See sqlite3ExprSkipCollate, the same code on line 3235, and<br>
how COLLATE is stored in Expr tree (it replaces the original Expr making<br>
it to be COLLATE's chils). This is why here pList->nExpr is the same as<br>
part count.<br>
<br>
> + const char *coll_name = pListItem->pExpr->u.zToken;<br>
> + assert(coll_name != NULL);<br>
> + size_t coll_name_len = strlen(coll_name);<br>
> + struct coll *coll = coll_by_name(coll_name, coll_name_len);<br>
> +<br>
> + if (is_binary_collation(coll_name)) {<br>
> + part_def_tmp[i].coll_id = COLL_NONE;<br>
> + } else {<br>
> + assert(coll != NULL);<br>
<br>
20. Why? Collation can be not found. Use sqlite3GetCollSeq. But before do<br>
rebase on the latest 2.0. You branch looks very out of date. Many things<br>
were changed after the latest 1.10 into 2.0 merge.<br>
<br>
> + part_def_tmp[i].coll_id = coll->id;<br>
> + }<br>
> +<br>
> + if (opts->sql != NULL) {<br>
<br>
21. How can it be NULL?<br>
<br>
> + /*<br>
> + * If index part has a collation then<br>
> + * corresponding part of statement should<br>
> + * be changed from 'column_name,' to<br>
> + * 'column_name COLLATE collation_name,'.<br>
> + */<br>
> + opts->sql[strlen(opts->sql) - 1] = ' ';<br>
> + asprintf(&opts->sql,"%sCOLLATE %s,", opts->sql,<br>
> + coll_name);<br>
<br>
22. Here and in all others asprintf you have leak - you reset opts->sql with new<br>
malloc (called in asprintf), but do not free the original. And please, use region<br>
instead. You can accumulate SQL parts via multiple region_alloc + single<br>
region_join + strdup at the end. It makes leaks be impossible, and does not<br>
require any malloc + free.<br>
<br>
> + }<br>
> + }<br>
> + if (pListItem->pExpr->op != TK_COLUMN &&<br>
> + pListItem->pExpr->op != TK_COLLATE) {<br>
> + printf("error %d\n", pListItem->pExpr->op);<br>
> + }<br>
> + }<br>
> +<br>
> + if (idxType == SQLITE_IDXTYPE_APPDEF) {<br>
> + opts->sql[strlen(opts->sql) - 1] = ')';<br>
> + }<br>
> +<br>
> + struct key_part_def *part_def = (struct key_part_def *)<br>
> + malloc(sizeof(*part_def) * part_count + 1);<br>
> +<br>
> + memcpy(part_def, part_def_tmp, sizeof(*part_def) * part_count);<br>
> + free(part_def_tmp);<br>
> +<br>
> + struct key_def *key_def = key_def_new_with_parts(part_def, part_count);<br>
> + assert(key_def != NULL);<br>
<br>
23. All this things can be removed by usage of key_def_new +<br>
multiple key_def_set_part.<br>
<br>
> +<br>
> + struct key_def *pk_key_def;<br>
> + if (idxType == SQLITE_IDXTYPE_APPDEF) {<br>
> + pk_key_def = space_index_key_def(space, 0);<br>
<br>
24. You must not use a space here.<br>
<br>
> + } else {<br>
> + pk_key_def = NULL;<br>
> + }<br>
> +<br>
> + enum index_type default_type = TREE;<br>
> +<br>
> + printf("space id %d index name %s index iid %d\n", space_def->id, name, iid);<br>
> + printf("nullables\n");<br>
> + for (uint32_t j = 0; j < key_def->part_count; j++){<br>
> + printf("%d\n", key_part_is_nullable(&key_def->parts[i]));<br><br>
</div>
<base target="_self" href="https://e.mail.ru/">
</div>
</div>
</div>
</blockquote>
Fixed all of the comments' issues.<br>
<br>-- <br>Ivan Koptelov<br></BODY></HTML>