Hello Sergei, Sir actually I added flags for unique_hash and index_hash for KEY these are
#define HA_INDEX_HASH 4 #define HA_UNIQUE_HASH 8 but ./mtr inoodb test are failing bacause KEY flag is 40 which is HA_BINARY_PACK_KEY + HA_UNIQUE_HASH but HA_UNIQUE_HASH should not be there . Is 4 and 8 are already used ,or should i go for another approach. Sir please review the code. Regards sachin On Mon, Jun 6, 2016 at 5:00 PM, Sergei Golubchik <s...@mariadb.org> wrote: > Hi, Sachin! > > On Jun 06, Sachin Setia wrote: >> Hello Sir >> Weekly Report for second week of gsoc >> 1. Implemented unique in the case of null >> 2. Implemented unique(A,b,c....) >> >> Currently working on >> tests , field hiding > > Good, thanks! > > I'd suggest to postpone field hiding until bb-10.2-default > branch is merged into 10.2. > > Start with tests, it's very important to have them. > See mysql-test/t/*.test and the manual is here: > https://mariadb.com/kb/en/mariadb/mysql-test/ > Don't forget direct tests of your HASH function. > > Then, may be, look at UPDATE. > > And here's a code review up to the commit b69e141 > >> diff --git a/sql/handler.cc b/sql/handler.cc >> index fb2921f..e68e37b 100644 >> --- a/sql/handler.cc >> +++ b/sql/handler.cc >> @@ -5863,16 +5863,86 @@ int handler::ha_reset() >> DBUG_RETURN(reset()); >> } >> >> +/** @breif > > typo: brief > >> + Compare two records >> + Need a better place for this function >> + @returns true if equal else false*/ >> +my_bool rec_hash_cmp(TABLE *tbl ,Field * hash_field) > > in C++ you can use 'bool'. 'my_bool' is only for C > > and, please, pay attention to where you put spaces :) > >> +{ >> + Item * t_item; >> + t_item=hash_field->vcol_info->expr_item->next; > > No, please don't rely on the 'next' pointer. It used to link > all items into a list, so that they could be destroyed all at once. > It's not how you traverse expression arguments. > > Use, for example, > > Item **arg,**arg_end; > for (arg=args, arg_end=args+arg_count; arg != arg_end ; arg++) > { > Item t_item= *arg; > > or > for (uint i= 0; i < arg_count; i++) > { > Item *t_item= args[i]; > > See these and other examples in item_func.cc > >> + int diff = tbl->record[1]-tbl->record[0]; > > record[1] - record[0] is always table->s->rec_buff_length > But there's one problem with that - you cannot use record[1], > see a comment below. > >> + Field * t_field; >> + int first_length,second_length; >> + while(t_item) >> + { >> + for(int i=0;i<tbl->s->fields;i++) >> + { >> + t_field=tbl->field[i]; >> + if(!my_strcasecmp(system_charset_info, >> + t_item->name,t_field->field_name)) >> + break; >> + } > > your t_item is an Item_field, so your t_field > will be t_item->field, no need to iterate anything. > >> + /* First check for nulls */ >> + if(t_field->is_real_null()||t_field->is_real_null(diff)) >> + { >> + return false; >> + } > > This shouldn't be necessary, see below, you shouldn't call this function > when a field value is NULL. > >> >> + /* >> + length of field is not equal to pack length when it is >> + subclass of field_blob and field _varsting >> + */ >> + first_length = t_field->data_length(); >> + second_length = t_field->data_length(diff); >> + if(first_length!=second_length) >> + return false; >> + if(t_field->cmp_max(t_field->ptr,t_field->ptr+diff,first_length)) >> + return false; >> + t_item=t_item->next; >> + } >> + return true; >> +} >> int handler::ha_write_row(uchar *buf) >> { >> - int error; >> + int error,map; >> Log_func *log_func= Write_rows_log_event::binlog_row_logging_function; >> + Field **field_iter; >> DBUG_ASSERT(table_share->tmp_table != NO_TMP_TABLE || >> m_lock_type == F_WRLCK); >> DBUG_ENTER("handler::ha_write_row"); >> DEBUG_SYNC_C("ha_write_row_start"); >> + /* First need to whether inserted record is unique or not */ >> >> + /* One More Thing if i implement hidden field then detection can be easy >> */ >> + field_iter=table->field; >> + for(uint i=0;i<table->s->fields;i++) > > You can iterate only virtual fields, there's a separate list for them. > >> + { >> + if(strncmp((*field_iter)->field_name,"DB_ROW_HASH_",12)==0) >> + { >> + table->file->ha_index_init(0,0); > > this might be a problem, as it changes the current index. > I think it's ok for INSERT, but may be not ok for UPDATE > (imagine, UPDATE ... WHERE indexed_column=5, the server does > ha_index_init for indexed_column, and then, in a loop, > ha_index_next/ha_update_row. So you cannot swicth to another index > in the middle of UPDATE loop) > > So, use ha_index_read_idx_map(), it doesn't change the current index. > > by the way, how could it be ha_index_init(0,0) ??? > you need to specify your unique key number, not 0. > >> + /* We need to add the null bit */ >> + /* If the column can be NULL, then in the first byte we put 1 if the >> + field value is NULL, 0 otherwise. */ >> + uchar * ptr = (uchar *)my_malloc(sizeof(char ) *1+8, MYF(MY_WME)); > > 1. where do you free it? > 2. doing malloc/free for _every row_ is too expensive, mallocs > generally are not very good for concurrency. > You can simply use > > uchar ptr[9] > > can you not? > >> + *ptr=0; > > why *ptr=0 ? You need to check whether the column value is actually NULL, > because now you set the key to "not null" unconditionally. > Besides, because NULL value can never cause HA_ERR_FOUND_DUPP_KEY, > you don't need to search if the column is NULL, you can skip this whole > check. > >> + ptr++; >> + memcpy(ptr,(*field_iter)->ptr,8); >> + ptr--; > > No need to go low-level, you can do: Why wouldn't you use > key_copy() function? Like > > uchar buf[9]; > DBUG_ASSERT(key->length == sizeof(buf)); > key_copy(ptr, buf, key, 0, false); > >> + map= >> table->file->ha_index_read_map(table->record[1],ptr,HA_WHOLE_KEY,HA_READ_KEY_EXACT); > > 1. why 'map'? what does it mean? > 2. Now it's getting a bit tricky. in UPDATE you cannot use record[1] - > it's the row before the update. I'm afraid you'll need to allocate another > table->s->rec_buff_length buffer for this, I don't see any other choice. > Of course, you can do it on demand, on the table's memroot, preserve > between calls, and so on. > >> + if(!map) >> + { >> + if(rec_hash_cmp(table,*field_iter)) >> + { >> + table->file->ha_index_end(); >> + DBUG_RETURN(HA_ERR_FOUND_DUPP_KEY); >> + } >> + >> + } >> + table->file->ha_index_end(); >> + } >> + field_iter++; >> + } >> MYSQL_INSERT_ROW_START(table_share->db.str, table_share->table_name.str); >> mark_trx_read_write(); >> increment_statistics(&SSV::ha_write_count); >> diff --git a/sql/item_func.cc b/sql/item_func.cc >> index e5c1f4c..fb23495 100644 >> --- a/sql/item_func.cc >> +++ b/sql/item_func.cc >> @@ -1916,6 +1916,41 @@ void Item_func_int_div::fix_length_and_dec() >> } >> >> >> +longlong Item_func_hash::val_int() >> +{ >> + unsigned_flag =true; > > the other way around, no space before, one space after :) > >> + ulong nr1= 1,temp=1, nr2= 0; >> + CHARSET_INFO *cs; >> + for(int i=0;i<arg_count;i++) >> + { >> + String * str = args[i]->val_str(); >> + if(args[i]->null_value) >> + { >> + null_value=1; > > don't forget to set null_value=0 if no arguments are NULL > >> + return 0; >> + } >> + nr2=args[i]->max_length; > > Hmm, I don't know. This makes your hash depend on metadata, not only on data. > I don't think it's a good idea. > >> + cs=str->charset(); >> + char l[4]; >> + int4store(l,str->length()); >> + cs->coll->hash_sort(cs, (uchar *)l,sizeof(str->length()), &temp, &nr2); > > not sizeof(str->length()), but sizeof(l). > >> + nr1^=temp; >> + cs->coll->hash_sort(cs, (uchar *)str->ptr(),str->length(), &temp, &nr2); > > Why did you change the hashing function? &nr1 should be the 4th argument > of hash_sort(), no xors afterwards. > > If you change it, you need to prove mathematically that your version > is better. That's why I personally prefer to not to mess with it :) > >> + nr1^=temp; >> + } >> + return (longlong)nr1; >> +} >> + >> + >> +void Item_func_hash::fix_length_and_dec() >> +{ >> + maybe_null= 1; >> + null_value= 0; > > no need to set null_value here, the value is calculated > in val_int, not in fix_length_and_dec. > fix_length_and_dec is called once, at the very beginning of a query, > val_int is called for every row. And you need to set null_value=0 > for every row. > >> + decimals= 0; >> + max_length= 8; >> +} >> + >> + >> longlong Item_func_mod::int_op() >> { >> DBUG_ASSERT(fixed == 1); >> diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc >> index 9b4238b..2c90f3c 100644 >> --- a/sql/sql_insert.cc >> +++ b/sql/sql_insert.cc >> @@ -724,6 +723,20 @@ bool mysql_insert(THD *thd,TABLE_LIST *table_list, >> DBUG_RETURN(TRUE); >> } >> > > Please mark places you're going to fix later, e.g. > // TODO when we'll have hidden fields, this will be removed > >> + context= &thd->lex->select_lex.context; >> + Field ** f=table_list->table->field; >> + List<Item> * i_list = (List<Item> *)values_list.first_node()->info; >> + for(uint i=0;i<table_list->table->s->fields;i++) >> + { >> + if(strncmp((*f)->field_name,"DB_ROW_HASH_",12)==0) >> + { >> + i_list->push_back(new (thd->mem_root) >> + Item_default_value(thd,context),thd->mem_root); >> + } >> + f++; >> + } >> + List_iterator_fast<List_item> its(values_list); >> + >> lock_type= table_list->lock_type; >> >> THD_STAGE_INFO(thd, stage_init); >> diff --git a/sql/sql_table.cc b/sql/sql_table.cc >> index dfce503..ac2988f 100644 >> --- a/sql/sql_table.cc >> +++ b/sql/sql_table.cc >> @@ -3012,8 +3012,8 @@ int prepare_create_field(Create_field *sql_field, >> (sql_field->decimals << FIELDFLAG_DEC_SHIFT)); >> break; >> } >> - if (!(sql_field->flags & NOT_NULL_FLAG) || >> - (sql_field->vcol_info)) /* Make virtual columns allow NULL values */ >> + if (!(sql_field->flags & NOT_NULL_FLAG) ) >> + /* (sql_field->vcol_info)) Make virtual columns allow NULL values */ > > don't forget to test (see above about mysql-test) that your change > does not affect normal generated columns and they are still always nullable. > to test that, create a table with a persistent generated column > with an expression that can return NULL. > >> sql_field->pack_flag|= FIELDFLAG_MAYBE_NULL; >> if (sql_field->flags & NO_DEFAULT_VALUE_FLAG) >> sql_field->pack_flag|= FIELDFLAG_NO_DEFAULT; >> @@ -3223,6 +3223,80 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO >> *create_info, >> bool tmp_table= create_table_mode == C_ALTER_TABLE; >> DBUG_ENTER("mysql_prepare_create_table"); >> >> + /* >> + scan the the whole alter list >> + and add one field if length of blob is zero >> + */ > > you need a TODO marker here (TODO change to work for all too-long unique keys) > >> + >> + List_iterator<Key> key_iter(alter_info->key_list); >> + Key *key_iter_key; >> + Key_part_spec *temp_colms; >> + char num = '1'; >> diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy >> index 24dc3c4..0135d9c 100644 >> --- a/sql/sql_yacc.yy >> +++ b/sql/sql_yacc.yy >> @@ -9543,6 +9543,15 @@ function_call_keyword: >> if ($$ == NULL) >> MYSQL_YYABORT; >> } >> + /*Currently hash for just 1 field >> + * */ > > your comment is already obsolete :) > >> + |HASH_SYM '(' expr_list ')' >> + { >> + $$= new (thd->mem_root)Item_func_hash(thd,*$3); >> + if($$==NULL) >> + MYSQL_YYABORT; >> + } >> + >> | INSERT '(' expr ',' expr ',' expr ',' expr ')' >> { >> $$= new (thd->mem_root) Item_func_insert(thd, $3, $5, $7, $9); > > Regards, > Sergei > Chief Architect MariaDB > and secur...@mariadb.org _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp