Thanks Guilhem, patches look good, only some nitpicks.
-- didrik
-static int sort_keyuse(KEYUSE *a,KEYUSE *b);
-static void set_position(JOIN *join,uint index,JOIN_TAB *table,KEYUSE *key);
-static bool create_ref_for_key(JOIN *join, JOIN_TAB *j, KEYUSE *org_keyuse,
+static int sort_keyuse(Key_use *a, Key_use *b);
+static void set_position(JOIN *join, uint index, JOIN_TAB *table,
+ Key_use *key);
No need to split this in two lines.
- bool null_rejecting;
- bool *cond_guard; /* See KEYUSE::cond_guard */
- uint sj_pred_no; /* See KEYUSE::sj_pred_no */
+ bool null_rejecting;
+ bool *cond_guard; ///< @sa Key_use::cond_guard
+ uint sj_pred_no; ///< @sa Key_use::sj_pred_no
(setq comment-column 48)
+ TABLE *table; ///< table owning the index
+ Item *val; ///< other side of the equality, or value if no field
+ table_map used_tables; ///< tables used on other side of equality
+ uint key; ///< number of index
+ uint keypart; ///< used part of the index
+ uint optimize; ///< 0, or KEY_OPTIMIZE_*
+ key_part_map keypart_map; ///< like keypart, but as a bitmap
+ ha_rows ref_table_rows; ///< Estimate of how many rows for a key value
Thanks for commenting, could you try to align (most of) them?
On Thu, Nov 4, 2010 at 6:11 PM, Guilhem Bichot <guilhem.bichot@stripped>wrote:
> Hello Jorgen and Tor,
>
> Tor Didriksen a écrit, Le 04.11.2010 13:27:
>
>>
>>
>> On Wed, Nov 3, 2010 at 2:54 PM, Guilhem Bichot <guilhem@stripped<mailto:
>> guilhem@stripped>> wrote:
>>
>> #At file:///home/mysql_src/bzrrepos_new/mysql-next-mr-bugfixing2/
>> based on revid:guilhem@stripped
>>
>> 3330 Guilhem Bichot 2010-11-03
>> Fix for BUG#57928 "More Valgrind warnings in print_keyuse()
>> with --debug":
>> for a fulltext index, the KEYUSE object had some uninitialized
>> members.
>> === modified file 'sql/sql_select.cc'
>> --- a/sql/sql_select.cc 2010-10-21 13:30:19 +0000
>> +++ b/sql/sql_select.cc 2010-11-03 13:54:05 +0000
>> @@ -5861,20 +5861,15 @@ add_key_part(DYNAMIC_ARRAY *keyuse_array
>> {
>> if (field->eq(form->key_info[key].key_part[part].field))
>> {
>> - KEYUSE keyuse;
>> - keyuse.table= field->table;
>> - keyuse.val= key_field->val;
>> - keyuse.used_tables= key_field->val->used_tables();
>> - keyuse.key= key;
>> - keyuse.keypart= part;
>> - keyuse.optimize= key_field->optimize &
>> KEY_OPTIMIZE_REF_OR_NULL;
>> - keyuse.keypart_map= (key_part_map) 1 << part;
>> - /* This will be set accordingly in optimize_keyuse */
>> - keyuse.ref_table_rows= ~(ha_rows) 0;
>> - keyuse.null_rejecting= key_field->null_rejecting;
>> - keyuse.cond_guard= key_field->cond_guard;
>> - keyuse.sj_pred_no= key_field->sj_pred_no;
>> - if (insert_dynamic(keyuse_array, (uchar*) &keyuse))
>> + const KEYUSE keyuse(field->table, key_field->val,
>> + key_field->val->used_tables(), key,
>> part,
>> + key_field->optimize &
>> KEY_OPTIMIZE_REF_OR_NULL,
>> + (key_part_map) 1 << part,
>> + ~(ha_rows) 0, // will be set in
>> optimize_keyuse
>> + key_field->null_rejecting,
>> + key_field->cond_guard,
>> + key_field->sj_pred_no);
>> + if (insert_dynamic(keyuse_array, &keyuse))
>> return TRUE;
>> }
>> }
>>
>>
>> I prefer
>>
>> const KEYUSE keyuse(field->table,
>> key_field->val,
>> key_field->val->used_tables(),
>> key,
>> part,
>> key_field->optimize &
>> KEY_OPTIMIZE_REF_OR_NULL,
>> (key_part_map) 1 << part,
>> ~(ha_rows) 0, // will be set in
>> optimize_keyuse
>> key_field->null_rejecting,
>> key_field->cond_guard,
>> key_field->sj_pred_no);
>>
>
> When I review, I rather annoy the developer asking him to squeeze on one
> line :-) Ok, the next patch will have one parameter per line.
>
>
>
>>
>> @@ -5938,16 +5933,11 @@ add_ft_keys(DYNAMIC_ARRAY *keyuse_array,
>> !(usable_tables & cond_func->table->map))
>> return FALSE;
>>
>> - KEYUSE keyuse;
>> - keyuse.table= cond_func->table;
>> - keyuse.val = cond_func;
>> - keyuse.key = cond_func->key;
>> - keyuse.keypart= FT_KEYPART;
>> - keyuse.used_tables=cond_func->key_item()->used_tables();
>> - keyuse.optimize= 0;
>> - keyuse.keypart_map= 0;
>> - keyuse.sj_pred_no= UINT_MAX;
>> - return insert_dynamic(keyuse_array,(uchar*) &keyuse);
>> + const KEYUSE keyuse(cond_func->table, cond_func,
>> + cond_func->key_item()->used_tables(),
>> cond_func->key,
>> + FT_KEYPART, 0, 0, ~(ha_rows) 0, false, NULL,
>> + UINT_MAX);
>> + return insert_dynamic(keyuse_array, &keyuse);
>> }
>>
>>
>> I would *strongly* prefer
>>
>> const KEYUSE keyuse(cond_func->table,
>> cond_func,
>> cond_func->key_item()->used_tables(),
>> cond_func->key,
>> FT_KEYPART,
>> 0, // optimize
>> 0, // keypart_map
>> ~(ha_rows) 0, // ref_table_rows
>> false, // null_rejecting
>> NULL, // cond_guard
>> UINT_MAX); // sj_pred_no
>>
>
> done
>
>
> === modified file 'sql/sql_select.h'
>> --- a/sql/sql_select.h 2010-09-23 12:16:36 +0000
>> +++ b/sql/sql_select.h 2010-11-03 13:54:05 +0000
>> @@ -39,17 +39,34 @@
>> #define KEY_OPTIMIZE_EXISTS 1
>> #define KEY_OPTIMIZE_REF_OR_NULL 2
>>
>> -typedef struct keyuse_t {
>> +/**
>> + @note such objects are stored in DYNAMIC_ARRAY which uses
>> sizeof(), so keep
>> + this class as POD as possible.
>> +*/
>> +class KEYUSE {
>> +public:
>> + KEYUSE(TABLE *table_arg, Item *val_arg, table_map used_tables_arg,
>> + uint key_arg, uint keypart_arg, uint optimize_arg,
>> + key_part_map keypart_map_arg, ha_rows ref_table_rows_arg,
>> + bool null_rejecting_arg, bool *cond_guard_arg,
>> + uint sj_pred_no_arg) :
>> + table(table_arg), val(val_arg), used_tables(used_tables_arg),
>> key(key_arg),
>> + keypart(keypart_arg), optimize(optimize_arg),
>> keypart_map(keypart_map_arg),
>> + ref_table_rows(ref_table_rows_arg),
>> null_rejecting(null_rejecting_arg),
>> + cond_guard(cond_guard_arg), sj_pred_no(sj_pred_no_arg)
>> {}
>>
>>
>> Please put the empty body on a separate line.
>>
>
> done
>
>
> And if you want to squish as many character as possible on each line,
>>
>
> oh yes!
>
>
> could you keep the argument list, and the initializers aligned?
>> something like:
>>
>> KEYUSE(TABLE *table_arg, Item *val_arg, table_map used_tables_arg,
>> uint key_arg, uint keypart_arg, uint optimize_arg,
>> key_part_map keypart_map_arg, ha_rows ref_table_rows_arg,
>> bool null_rejecting_arg, bool *cond_guard_arg,
>> uint sj_pred_no_arg) :
>> table(table_arg), val(val_arg), used_tables(used_tables_arg),
>> key(key_arg), keypart(keypart_arg), optimize(optimize_arg),
>> keypart_map(keypart_map_arg), ref_table_rows(ref_table_rows_arg),
>> null_rejecting(null_rejecting_arg), cond_guard(cond_guard_arg),
>> sj_pred_no(sj_pred_no_arg)
>> {}
>>
>
> done
>
>
> + /** Estimate of how many rows for a key value */
>>
>>
>> The style guide was recently extended (at your request :-) to allow
>> // Estimate of how many rows for a key value.
>>
>
> Changed to /// (doxygen-compliant).
>
>
> Now we have a mix of five different comment styles, just for the member
>> fields of this class.
>>
>
> I will unify a bit the comment styles for those members in the next patch.
>
> I'm quite happy to be allowed to write
> // comment on a line even with no code on the left
> now. Thanks a lot!
>
> Also renamed KEYUSE to Key_use. That led to changes in many lines; I then
> used that as pretext to make those lines compliant with our coding style,
> deleting white space, adding space after equal, and other little things; so
> the new commit will be bigger than the first one.
> The new commit is in two patches:
> http://lists.mysql.com/commits/122859
> changes insert_dynamic/set_dynamic;
> then
> http://lists.mysql.com/commits/122862
> is incremental on it and fixes the bug.
>
>
