Hi Shubham, On Sun, Jun 5, 2016 at 9:25 AM, Sergei Golubchik <[email protected]> wrote:
> Hi, Shubham! > > Here's a first review. > > Summary: looks good so far, main comments: > > 1. Coding style. Use the same coding style as everywhere else in the > file you're editing. It's the same in sql/ and in myisam/ directories. > But InnoDB uses different coding style. > You can refer to the link below for coding guidelines. https://dev.mysql.com/doc/internals/en/general-development-guidelines.html Also, if you use vim, there are some good vim suggestions to fix indentations. Best, Nirbhay > > 2. Tests, tests, tests! Please start adding tests now, and commit them > into your branch. They should be somewhere under mysql-test/ directory. > The full documentation is here: > https://mariadb.com/kb/en/mariadb/mysqltest/ > But even without it you can create your tests by starting of from > existing test files. > > More comments below: > > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > > index dfce503..31f282b 100644 > > --- a/sql/sql_table.cc > > +++ b/sql/sql_table.cc > > @@ -3876,8 +3876,9 @@ mysql_prepare_create_table(THD *thd, > HA_CREATE_INFO *create_info, > > column->length= MAX_LEN_GEOM_POINT_FIELD; > > if (!column->length) > > { > > - my_error(ER_BLOB_KEY_WITHOUT_LENGTH, MYF(0), > column->field_name.str); > > - DBUG_RETURN(TRUE); > > + key_info->algorithm=HA_KEY_ALG_HASH; > > There's more to it. The task title is, indeed, "unique index for > blobs", but the actual goal is to have unique indexes for anything > that is too long for normal btree indexes. That's what MI_UNIQUE does. > > so, you should support unique indexes also for blobs where column->length > is specified, but is too large. Or unique indexes for a combination of > ten long varchar columns, for example. > > So, you also need to patch the code where it issues ER_TOO_LONG_KEY. > > Also we'd want to use MI_UNIQUE for keys where a user has > explicitly specified USING HASH, but let's look at it later > > > + // my_error(ER_BLOB_KEY_WITHOUT_LENGTH, MYF(0), > column->field_name.str); > > + // DBUG_RETURN(TRUE); > > } > > } > > #ifdef HAVE_SPATIAL > > diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc > > index 72bc4c0..9aba216 100644 > > --- a/storage/myisam/ha_myisam.cc > > +++ b/storage/myisam/ha_myisam.cc > > @@ -216,44 +216,59 @@ static void mi_check_print_msg(HA_CHECK *param, > const char* msg_type, > > 0 OK > > !0 error code > > */ > > - > > int table2myisam(TABLE *table_arg, MI_KEYDEF **keydef_out, > > - MI_COLUMNDEF **recinfo_out, uint *records_out) > > + MI_COLUMNDEF **recinfo_out,MI_UNIQUEDEF > **uniquedef_out ,uint *records_out) > > { > > - uint i, j, recpos, minpos, fieldpos, temp_length, length; > > + uint i, j, recpos, minpos, fieldpos, temp_length, length, k, l, m; > > enum ha_base_keytype type= HA_KEYTYPE_BINARY; > > uchar *record; > > KEY *pos; > > MI_KEYDEF *keydef; > > + MI_UNIQUEDEF *uniquedef; > > MI_COLUMNDEF *recinfo, *recinfo_pos; > > HA_KEYSEG *keyseg; > > TABLE_SHARE *share= table_arg->s; > > uint options= share->db_options_in_use; > > DBUG_ENTER("table2myisam"); > > + pos= table_arg->key_info; > > + share->uniques=0; > > + for (i= 0; i < share->keys; i++,pos++) > > + { > > + if(pos->algorithm==HA_KEY_ALG_HASH) > > + { > > + share->uniques++ ; > > + } > > + } > > if (!(my_multi_malloc(MYF(MY_WME), > > recinfo_out, (share->fields * 2 + 2) * sizeof(MI_COLUMNDEF), > > - keydef_out, share->keys * sizeof(MI_KEYDEF), > > + keydef_out, (share->keys - share->uniques) * > sizeof(MI_KEYDEF), > > + uniquedef_out, share->uniques * sizeof(MI_UNIQUEDEF), > > &keyseg, > > (share->key_parts + share->keys) * sizeof(HA_KEYSEG), > > NullS))) > > DBUG_RETURN(HA_ERR_OUT_OF_MEM); /* purecov: inspected */ > > keydef= *keydef_out; > > recinfo= *recinfo_out; > > + uniquedef= *uniquedef_out; > > pos= table_arg->key_info; > > + k=0; > > + m=0; > > better call your indexes 'k' and 'u', for keydefs and uniques. > > > for (i= 0; i < share->keys; i++, pos++) > > { > > - keydef[i].flag= ((uint16) pos->flags & (HA_NOSAME | HA_FULLTEXT | > HA_SPATIAL)); > > - keydef[i].key_alg= pos->algorithm == HA_KEY_ALG_UNDEF ? > > + if(pos->algorithm!=HA_KEY_ALG_HASH) > > + { > > + keydef[k].flag= ((uint16) pos->flags & (HA_NOSAME | HA_FULLTEXT | > HA_SPATIAL)); > > + keydef[k].key_alg= pos->algorithm == HA_KEY_ALG_UNDEF ? > > (pos->flags & HA_SPATIAL ? HA_KEY_ALG_RTREE : HA_KEY_ALG_BTREE) : > > pos->algorithm; > > - keydef[i].block_length= pos->block_size; > > - keydef[i].seg= keyseg; > > - keydef[i].keysegs= pos->user_defined_key_parts; > > + keydef[k].block_length= pos->block_size; > > + keydef[k].seg= keyseg; > > + keydef[k].keysegs= pos->user_defined_key_parts; > > for (j= 0; j < pos->user_defined_key_parts; j++) > > { > > Field *field= pos->key_part[j].field; > > type= field->key_type(); > > - keydef[i].seg[j].flag= pos->key_part[j].key_part_flag; > > + keydef[k].seg[j].flag= pos->key_part[j].key_part_flag; > > > > if (options & HA_OPTION_PACK_KEYS || > > (pos->flags & (HA_PACK_KEY | HA_BINARY_PACK_KEY | > > @@ -266,52 +281,104 @@ int table2myisam(TABLE *table_arg, MI_KEYDEF > **keydef_out, > > { > > /* No blobs here */ > > if (j == 0) > > - keydef[i].flag|= HA_PACK_KEY; > > + keydef[k].flag|= HA_PACK_KEY; > > if (!(field->flags & ZEROFILL_FLAG) && > > (field->type() == MYSQL_TYPE_STRING || > > field->type() == MYSQL_TYPE_VAR_STRING || > > ((int) (pos->key_part[j].length - field->decimals())) >= > 4)) > > - keydef[i].seg[j].flag|= HA_SPACE_PACK; > > + keydef[k].seg[j].flag|= HA_SPACE_PACK; > > } > > else if (j == 0 && (!(pos->flags & HA_NOSAME) || > pos->key_length > 16)) > > - keydef[i].flag|= HA_BINARY_PACK_KEY; > > + keydef[k].flag|= HA_BINARY_PACK_KEY; > > } > > - keydef[i].seg[j].type= (int) type; > > - keydef[i].seg[j].start= pos->key_part[j].offset; > > - keydef[i].seg[j].length= pos->key_part[j].length; > > - keydef[i].seg[j].bit_start= keydef[i].seg[j].bit_end= > > - keydef[i].seg[j].bit_length= 0; > > - keydef[i].seg[j].bit_pos= 0; > > - keydef[i].seg[j].language= field->charset_for_protocol()->number; > > + keydef[k].seg[j].type= (int) type; > > + keydef[k].seg[j].start= pos->key_part[j].offset; > > + keydef[k].seg[j].length= pos->key_part[j].length; > > + keydef[k].seg[j].bit_start= keydef[k].seg[j].bit_end= > > + keydef[k].seg[j].bit_length= 0; > > + keydef[k].seg[j].bit_pos= 0; > > + keydef[k].seg[j].language= field->charset_for_protocol()->number; > > > > if (field->null_ptr) > > { > > - keydef[i].seg[j].null_bit= field->null_bit; > > - keydef[i].seg[j].null_pos= (uint) (field->null_ptr- > > + keydef[k].seg[j].null_bit= field->null_bit; > > + keydef[k].seg[j].null_pos= (uint) (field->null_ptr- > > (uchar*) > table_arg->record[0]); > > } > > else > > { > > - keydef[i].seg[j].null_bit= 0; > > - keydef[i].seg[j].null_pos= 0; > > + keydef[k].seg[j].null_bit= 0; > > + keydef[k].seg[j].null_pos= 0; > > } > > if (field->type() == MYSQL_TYPE_BLOB || > > field->type() == MYSQL_TYPE_GEOMETRY) > > { > > - keydef[i].seg[j].flag|= HA_BLOB_PART; > > + keydef[k].seg[j].flag|= HA_BLOB_PART; > > /* save number of bytes used to pack length */ > > - keydef[i].seg[j].bit_start= (uint) (field->pack_length() - > > + keydef[k].seg[j].bit_start= (uint) (field->pack_length() - > > portable_sizeof_char_ptr); > > } > > else if (field->type() == MYSQL_TYPE_BIT) > > { > > - keydef[i].seg[j].bit_length= ((Field_bit *) field)->bit_len; > > - keydef[i].seg[j].bit_start= ((Field_bit *) field)->bit_ofs; > > - keydef[i].seg[j].bit_pos= (uint) (((Field_bit *) > field)->bit_ptr - > > + keydef[k].seg[j].bit_length= ((Field_bit *) field)->bit_len; > > + keydef[k].seg[j].bit_start= ((Field_bit *) field)->bit_ofs; > > + keydef[k].seg[j].bit_pos= (uint) (((Field_bit *) > field)->bit_ptr - > > (uchar*) > table_arg->record[0]); > > } > > } > > keyseg+= pos->user_defined_key_parts; > > + k++; > > + } > > + else > > + { > > + uniquedef[m].sql_key_no=i; > > + uniquedef[m].null_are_equal=1; > > + uniquedef[m].seg=keyseg; > > please follow code formatting rules that you can see elsewhere in this file > > > + uniquedef[m].keysegs= pos->user_defined_key_parts; > > + for (l= 0; l < pos->user_defined_key_parts; l++) > > + { > > + Field *field= pos->key_part[l].field; > > + type= field->key_type(); > > + uniquedef[m].seg[l].flag= pos->key_part[l].key_part_flag; > > + uniquedef[m].seg[l].type= (int) type; > > + uniquedef[m].seg[l].start= pos->key_part[l].offset; > > + uniquedef[m].seg[l].length= pos->key_part[l].length; > > + uniquedef[m].seg[l].bit_start= uniquedef[m].seg[l].bit_end= > > + uniquedef[m].seg[l].bit_length= 0; > > + uniquedef[m].seg[l].bit_pos= 0; > > + uniquedef[m].seg[l].language= > field->charset_for_protocol()->number; > > + > > + if (field->null_ptr) > > + { > > + uniquedef[m].seg[l].null_bit= field->null_bit; > > + uniquedef[m].seg[l].null_pos= (uint) (field->null_ptr- > > + (uchar*) > table_arg->record[0]); > > + } > > + else > > + { > > + uniquedef[m].seg[l].null_bit= 0; > > + uniquedef[m].seg[l].null_pos= 0; > > + } > > + if (field->type() == MYSQL_TYPE_BLOB || > > + field->type() == MYSQL_TYPE_GEOMETRY) > > + { > > + uniquedef[m].seg[l].flag|= HA_BLOB_PART; > > + /* save number of bytes used to pack length */ > > + uniquedef[m].seg[l].bit_start= (uint) (field->pack_length() - > > + portable_sizeof_char_ptr); > > + } > > + else if (field->type() == MYSQL_TYPE_BIT) > > + { > > + uniquedef[m].seg[l].bit_length= ((Field_bit *) field)->bit_len; > > + uniquedef[m].seg[l].bit_start= ((Field_bit *) field)->bit_ofs; > > + uniquedef[m].seg[l].bit_pos= (uint) (((Field_bit *) > field)->bit_ptr - > > + (uchar*) > table_arg->record[0]); > > + } > > this seems to be the same code as for keydefs, isn't it? > better to extract it into a separate function. Like: > > setup_keyparts(pos, keydef[k].seg); > setup_keyparts(pos, uniquedef[m].seg); > > > + > > + } > > + keyseg+= pos->user_defined_key_parts; > > + m++; > > + } > > } > > if (table_arg->found_next_number_field) > > keydef[share->next_number_index].flag|= HA_AUTO_KEY; > > @@ -453,6 +528,7 @@ int check_definition(MI_KEYDEF *t1_keyinfo, > MI_COLUMNDEF *t1_recinfo, > > MI_KEYDEF *t2_keyinfo, MI_COLUMNDEF *t2_recinfo, > > uint t2_keys, uint t2_recs, bool strict, TABLE > *table_arg) > > { > > + return 0; > > to be fixed, I suppose? > > > uint i, j; > > DBUG_ENTER("check_definition"); > > my_bool mysql_40_compat= table_arg && table_arg->s->frm_version < > FRM_VER_TRUE_VARCHAR; > > diff --git a/storage/myisam/mi_write.c b/storage/myisam/mi_write.c > > index ff96ee8..4d2b62a 100644 > > --- a/storage/myisam/mi_write.c > > +++ b/storage/myisam/mi_write.c > > @@ -188,6 +189,28 @@ int mi_write(MI_INFO *info, uchar *record) > > mi_flush_bulk_insert(info, j); > > } > > info->errkey= (int) i; > > + prev=-1; > > + keydef_no=0; > > + for (k=0 ; k < share->state.header.uniques ; k++) > > + { > > + MI_UNIQUEDEF *def= share->uniqueinfo + k; > > + > > + if(keydef_no < (int) i+1) > > + { > > + diff= ((int)def->sql_key_no) - prev -1; > > + keydef_no += diff; > > + } > > + prev= (int) def->sql_key_no; > > + if(keydef_no <= (int) i+1) > > + { > > + info->errkey += 1; > > + } > > + else > > + break; > > this can be done a bit simpler, without diff, prev, or keydef_no: > > for (k=0; k < share->state.header.uniques; k++, i++) > if (i < share->uniqueinfo[k].sql_key_no) > break; > info->errkey= (int) i; > > > + > > + } > > + > > + > > while ( i-- > 0) > > { > > if (mi_is_key_active(share->state.key_map, i)) > > Regards, > Sergei > Chief Architect MariaDB > and [email protected] > > _______________________________________________ > Mailing list: https://launchpad.net/~maria-developers > Post to : [email protected] > Unsubscribe : https://launchpad.net/~maria-developers > More help : https://help.launchpad.net/ListHelp >
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
