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