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 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;ival_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;itable->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 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 security@mariadb.org