- developers - lists.mariadb.org

[Maria-developers] Please review MDEV-11357 Split Item_cache::get_cache() into virtual methods in Type_handler
by Alexander Barkov 26 Nov '16

26 Nov '16

Hello Sanja, Can you please review a patch for MDEV-11357? This is another per-requisite task for: MDEV-4912 Add a plugin to field types (column types) Thanks!

1 0

[Maria-developers] MDEV-11297: Re why tree_delete doesn't work in Item_func_group_concat code
by Sergey Petrunia 25 Nov '16

25 Nov '16

Hi Varun, Part#1 of the followup to our discussion about removing from the TREE object. So I tried to get tree_remove() to work. - I enabled deletion from the tree based on your analysis. - then, I got the tree_remove code to compile by making these changes: diff --git a/sql/item_sum.cc b/sql/item_sum.cc index e137720..8bcd7c6 100644 --- a/sql/item_sum.cc +++ b/sql/item_sum.cc @@ -3835,10 +3835,13 @@ bool Item_func_group_concat::add() return 1; if(limit_clause && (tree->elements_in_tree > row_limit+offset_limit)) { - //tree_search_edge(tree,tree->parents,&tree->parents, - //offsetof(TREE_ELEMENT, left)); - //tree_delete(tree, table->record[0] + table->s->null_bytes, 0, - // tree->custom_arg); + TREE_ELEMENT *parents[MAX_TREE_HEIGHT+1]; + TREE_ELEMENT **pos; + void *key; + key= tree_search_edge(tree, parents, &pos, + offsetof(TREE_ELEMENT, left)); + uint tree_key_length= table->s->reclength - table->s->null_bytes; + tree_delete(tree, key, tree_key_length, tree->custom_arg); } } /* @@ -4036,7 +4039,10 @@ bool Item_func_group_concat::setup(THD *thd) thd->variables.sortbuff_size/16), 0, tree_key_length, group_concat_key_cmp_with_order, NULL, (void*) this, - MYF(MY_THREAD_SPECIFIC)); + //MYF(MY_THREAD_SPECIFIC)); + limit_clause? MYF(MY_TREE_WITH_DELETE) : MYF(0)); + // psergey-note: it passes MY_THREAD_SPECIFIC, while init_tree + // actually checks for MY_TREE_WITH_DELETE! } if (distinct) It still didn't remove the element! I started debugging, ended up here: #0 group_concat_key_cmp_with_order (arg=0x7fffc78604d0, key1=0x7fffc78595a8, key2=0x7fffc78595a8) at /home/psergey/dev-git/10.2-varun/sql/item_sum.cc:3464 #1 0x00005555563d54e3 in tree_delete (tree=0x7fffc7860620, key=0x7fffc78595a8, key_size=11, custom_arg=0x7fffc78604d0) at /home/psergey/dev-git/10.2-varun/mysys/tree.c:293 #2 0x0000555555e14546 in Item_func_group_concat::add (this=0x7fffc78604d0) at /home/psergey/dev-git/10.2-varun/sql/item_sum.cc:3844 #3 0x0000555555e15b8b in Aggregator_simple::add (this=0x7fffc78632a8) at /home/psergey/dev-git/10.2-varun/sql/item_sum.h:679 #4 0x0000555555b52e89 in Item_sum::aggregator_add (this=0x7fffc78604d0) at /home/psergey/dev-git/10.2-varun/sql/item_sum.h:527 #5 0x0000555555b48a98 in update_sum_func (func_ptr=0x7fffc78619b0) at /home/psergey/dev-git/10.2-varun/sql/sql_select.cc:23240 and the code at the end of group_concat_key_cmp_with_order says: /* We can't return 0 because in that case the tree class would remove this item as double value. This would cause problems for case-changes and if the returned values are not the same we do the sort on. */ return 1; So, group_concat_key_cmp_with_order() never returns 0, which means tree_delete() can't find the element it wants to delete, and that's why no element is ever deleted. (The rest of discussion is coming in separate emails). BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] MDEV-11337 Split Item::save_in_field() into virtual methods in Type_handler
by Alexander Barkov 25 Nov '16

25 Nov '16

Hello Nirbhay, Can you please review a patch for 10.3: MDEV-11337 Split Item::save_in_field() into virtual methods in Type_handler It also automatically fixed two problems: MDEV-11331 Wrong result for INSERT INTO t1 (datetime_field) VALUES (hybrid_function_of_TIME_data_type) MDEV-11333 Expect "Impossible where condition" for WHERE timestamp_field>=DATE_ADD(TIMESTAMP'9999-01-01 00:00:00',INTERVAL 1000 YEAR) because the new code is now symmetric for all data types. Thanks!

2 2

[Maria-developers] Please review MDEV-11347Move add_create_index_prepare(), add_key_to_list(), set_trigger_new_row(), set_local_variable(), set_system_variable(), create_item_for_sp_var() as methods to LEX
by Alexander Barkov 24 Nov '16

24 Nov '16

Hello Sanja, Please review a patch for MDEV-11347. It turns a few other function into methods in LEX, to be able to reuse them easier. Thanks!

1 0

[Maria-developers] MDEV-11344 Split Arg_comparator::set_compare_func() into virtual methods in Type_handler
by Alexander Barkov 24 Nov '16

24 Nov '16

Hello Sanja, Can you please review a patch for MDEV-11344? Thanks.

1 1

[Maria-developers] Please review MDEV-11330 Split Item_func_hybrid_field_type::val_xxx() into methods in Type_handler
by Alexander Barkov 22 Nov '16

22 Nov '16

Hello Vicențiu, Please review a patch for MDEV-11330. Thanks!

1 0

[Maria-developers] Please review MDEV-11298 also fixing GIS bugs MDEV-9405 and MDEV-9425
by Alexander Barkov 21 Nov '16

21 Nov '16

Hello Alexey, Please review a patch for: MDEV-11302 Add class Type_ext_attributes and Type_handler::join_type_ext_attributes() A detailed description can be found in the task ticket: https://jira.mariadb.org/browse/MDEV-11302 This patch also fixes the problems reported in: MDEV-9405 Hybrid functions, SP do not preserve geometry type MDEV-9425 Hybrid functions and UNION do not preserve spatial REF_SYSTEM_ID Some calls for geometry_type() and/or srid() were forgotten in the old code. The new code replaces calls for geometry_type() and srid() to a generic type_ext_attributes() and makes maintaining/adding of similar data type specific attributes easier. The new class Type_ext_attributes will be used in a few field creation methods in Type_handler later. Thanks!

1 1

Re: [Maria-developers] [Commits] 41a12f9: MDEV-8320 Allow index usage for DATE(datetime_column) = const.
by Sergey Petrunia 21 Nov '16

21 Nov '16

In-Reply-To: <20160928105123.6D948140DDC(a)nebo.localdomain> Hi Alexey, Thanks for your patience in waiting for the review. Please find it below. On Wed, Sep 28, 2016 at 02:50:19PM +0400, Alexey Botchkov wrote: > revision-id: 41a12f990519fb68eaa66ecc6860985471e6ba5a (mariadb-10.1.8-264-g41a12f9) > parent(s): 28f441e36aaaec15ce7d447ef709fad7fbc7cf7d > committer: Alexey Botchkov > timestamp: 2016-09-28 14:48:54 +0400 > message: > > MDEV-8320 Allow index usage for DATE(datetime_column) = const. > > Test for 'sargable functions' added. > First, t/range.test crashes after I apply the patch. MTR output is here: https://gist.github.com/spetrunia/d10165820664e0d18d4a667d44d226ee but I've got the crash on two different machines so should be easy to repeat > diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h > index 6d432bd..516bb07 100644 > --- a/sql/item_cmpfunc.h > +++ b/sql/item_cmpfunc.h > @@ -136,6 +136,14 @@ class Item_bool_func :public Item_int_func > { > protected: > /* > + Some functions modify it's arguments for the optimizer. > + So for example the condition 'Func(fieldX) = constY' turned into > + 'fieldX = cnuR(constY)' so that optimizer can use an index on fieldX. > + */ What's cnuR? Ok, I eventually got it, but the comments should not have such puzzles. > + Item *opt_args[3]; > + uint opt_arg_count; > + > + /* > +static Item_field *get_local_field (Item *field) > +{ > + Item *ri= field->real_item(); > + return (ri->type() == Item::FIELD_ITEM > + && !(field->used_tables() & OUTER_REF_TABLE_BIT) > + && !((Item_field *)ri)->get_depended_from()) ? (Item_field *) ri : 0; > +} Please fix indentation and add comments. Does this function do what is_local_field does, or there is some difference? > + > + > +static Item_field *field_in_sargable_func(Item *fn) > +{ > + fn= fn->real_item(); > + > + if (fn->type() == Item::FUNC_ITEM && > + strcmp(((Item_func *)fn)->func_name(), "cast_as_date") == 0) > + > + { > + Item_date_typecast *dt= (Item_date_typecast *) fn; > + return get_local_field(dt->arguments()[0]); > + } > + return 0; Please use NULL instead of 0, and !strcmp() instead of strcmp()=0. > @@ -5036,6 +5060,25 @@ Item_func_like::add_key_fields(JOIN *join, KEY_FIELD **key_fields, > } > > > +bool Item_bool_rowready_func2::add_extra_key_fields(THD *thd, > + JOIN *join, KEY_FIELD **key_fields, > + uint *and_level, > + table_map usable_tables, > + SARGABLE_PARAM **sargables) > +{ > + Item_field *f; > + if ((f= field_in_sargable_func(args[0])) && args[1]->const_item()) What is the difference between add_key_fields and add_extra_key_fields? Any cases where one should call one but not the other? Please also do indentation as coding style specifies. > diff --git a/sql/item_timefunc.cc b/sql/item_timefunc.cc > index 41dc967..3124444 100644 > --- a/sql/item_timefunc.cc > +++ b/sql/item_timefunc.cc > @@ -2569,6 +2569,39 @@ bool Item_date_typecast::get_date(MYSQL_TIME *ltime, ulonglong fuzzy_date) > } > > > +bool Item_date_typecast::create_reverse_func(enum Functype cmp_type, > + THD *thd, Item *r_arg, uint *a_cnt, Item** a) > +{ We need a specification of what exactly this function does, and a usage scenario in the comment. This function actually creates multiple (up to 3?) functions. If one has a condition DATE(t1.d) < '2000-01-04' then we get (gdb) p ((Item*)cond)->opt_arg_count $37 = 3 (gdb) p dbug_print_item(((Item*)cond)->opt_args[0]) $38 = 0x555557083e20 <dbug_item_print_buf> "t1.d" (gdb) p dbug_print_item(((Item*)cond)->opt_args[1]) $39 = 0x555557083e20 <dbug_item_print_buf> "day_begin('2000-01-19')" (gdb) p dbug_print_item(((Item*)cond)->opt_args[2]) $40 = 0x555557083e20 <dbug_item_print_buf> "day_end('2000-01-19')" which makes sense, but the description is lacking. Probably the name "create_reverse_func" is not good, because 1. multiple functions are created and 2. neither of them is the reverse. I can't suggest a better name at the moment, though. Let's both think about how to make this code cleared for an uninformed reader. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

2 1

Re: [Maria-developers] MDEV-11245 Move prepare_create_field and sp_prepare_create_field() as methods to Column_definition
by Alexander Barkov 20 Nov '16

20 Nov '16

Hi Sanja, On 11/20/2016 12:29 AM, Oleksandr Byelkin wrote: > Hi! > > Patch is OK to push. Thanks for reviewing! > Actually it is good sign that 'refefence->field' > turned to 'field' now! Yeah, this makes the code easier to read. Greetings! > > On Fri, Nov 18, 2016 at 12:43 PM, Alexander Barkov <bar(a)mariadb.org > <mailto:bar@mariadb.org>> wrote: > > Hello Sanja, > > can you please review a patch for MDEV-11245 for 10.3? > > It's a small refactoring intended to make patches for these tasks > look better: > > - MDEV-10577 sql_mode=ORACLE: %TYPE in variable declarations > - MDEV-10914 ROW data type for stored routine variables > > Thanks! > >

1 0

[Maria-developers] MDEV-11245 Move prepare_create_field and sp_prepare_create_field() as methods to Column_definition
by Alexander Barkov 18 Nov '16

18 Nov '16

Hello Sanja, can you please review a patch for MDEV-11245 for 10.3? It's a small refactoring intended to make patches for these tasks look better: - MDEV-10577 sql_mode=ORACLE: %TYPE in variable declarations - MDEV-10914 ROW data type for stored routine variables Thanks!

1 0