- developers - lists.mariadb.org

[Maria-developers] Review for MDEV-6112 multiple triggers per table
by Alexander Barkov 28 Sep '16

28 Sep '16

Hello Monty. I checked your patch for MDEV-6112 multiple triggers per table. Here are some quick notes. I'll send a detailed review separately. 1. It would be nice to push refactoring changes in separate patch: a. do sql_mode_t refactoring b. "trg_idx -> Trigger *" refactoring c. opt_debugging related things 2. Why not to reuse the FOLLOWING_SYM and PRECEDING_SYM keywords instead of adding new ones FOLLOWS_SYM, PRECEDES_SYM ? (this is a non-standard syntax anyway) 3. I'd prefer PRECEDES/FOLLOWS to go before FOR EACH ROW. "FOR EACH ROW stmt" is a kind of single clause responsible for the action. It's very confusing to have PRECEDES/FOLLOWS characteristics in between "FOR EACH ROW" and stmt. 4. You have a lot of changes related to trim_whitespace() where you declare an unused "prefix_removed" variable. It would be nice to avoid declaring it. Possible ways: a. Perhaps the new function version can be defined like this: size_t trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str); and make it return "prefix length removed", instead of adding a new parameter. b. Another option is just to overload it: extern void trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str, uint *prefix_removed); inline void trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str) { uint not_used; trim_whitespace(cs, str, &not_used); } 5. The "CREATED" field is not populated well. I'm getting strange values like 1970-04-24 09:35:00.30. Btw, his query returns a correct value: SELECT FROM_UNIXTIME(UNIX_TIMESTAMP(CREATED)*100) FROM INFORMATION_SCHEMA.triggers; You must have forgotten to multiply or delete to 100 somewhere. 6. In replication tests it's better to make sure that CREATED is safely replicated (instead of changing it to #). By the way, in other tests it's also a good idea not to use # for CREATED. (see the previous problem) You can use this: SET TIMESTAMP=UNIX_TIMESTAMP('2001-01-01 10:20:30'); ... SET TIMESTAM=DEFAULT; 7. I suggest to move this code into a method, say find_trigger(): + if (table->triggers) + { + Trigger *t; + for (t= table->triggers->get_trigger(TRG_EVENT_INSERT, + TRG_ACTION_BEFORE); + t; + t= t->next) + if (t->is_fields_updated_in_trigger(&full_part_field_set)) + DBUG_RETURN(false); + } It's repeated at least two times. 8. The following structure fields are defined two times: enum trigger_order_type ordering_clause; LEX_STRING anchor_trigger_name; The first time for st_trg_chistics. The second time for %union in sql_yacc.yy Note, there will be the third time in sql_yacc_ora.yy. Please define a structure in sql_lex.h instead: struct st_trg_execution_order_chistics { /** FOLLOWS or PRECEDES as specified in the CREATE TRIGGER statement. */ enum trigger_order_type ordering_clause; /** Trigger name referenced in the FOLLOWS/PRECEDES clause of the CREATE TRIGGER statement. */ LEX_STRING anchor_trigger_name; }; and reuse it in here: struct st_trg_chirstics: public st_trg_execution_order_chistics { ... }; and in here: %union { ... st_trg_execition_order_chistics trg_execution_order_chirstics; ... }; Please also rename trg_characteristics to trg_execution_order_chistics, because "trg_characteristics" is more suitable to something having type st_trg_chirstics rather than st_trg_execution_order_chistics. Greetings.

1 1

[Maria-developers] Possibly a problem in "Allowed to use WITH clauses before SELECT in CREATE ... SELECT and INSERT ... SELECT"
by Alexander Barkov 28 Sep '16

28 Sep '16

Hello Igor, I was rebasing bb-10.2-compatibility on top of the latest 10.2 and noticed this grammar change: > create_select_query_specification: > - SELECT_SYM create_select_part2 create_select_part3 create_select_part4 > + SELECT_SYM opt_with_clause create_select_part2 create_select_part3 > + create_select_part4 > + { > + Select->set_with_clause($2); > + } > ; I have some questions: - From my understanding, opt_with_clause should go before SELECT_SYM Why does it go after? - Why this grammar change is not covered in tests? - What is the MDEV number for this change? Thanks!

1 0

Re: [Maria-developers] [Commits] 13b5098: MDEV-9531: GROUP_CONCAT with ORDER BY inside takes a lot of memory while it's executed
by Sergei Golubchik 26 Sep '16

26 Sep '16

Hi, Oleksandr! On Jun 28, Oleksandr Byelkin wrote: > revision-id: 13b5098fcaa888173472d255e29aff22bcc5baae (mariadb-10.1.13-18-g13b5098) > parent(s): 732adec0a4c75d99389230feeb0deca0ad668de7 > committer: Oleksandr Byelkin > timestamp: 2016-06-28 10:59:59 +0200 > message: > > MDEV-9531: GROUP_CONCAT with ORDER BY inside takes a lot of memory while it's executed > > Limitation added to Red-Black tree. > > --- > include/my_tree.h | 14 +++- > mysql-test/r/group_concat_big.result | 6 ++ > mysql-test/t/group_concat_big.result | 6 ++ > mysql-test/t/group_concat_big.test | 6 ++ > mysys/tree.c | 156 ++++++++++++++++++++++++----------- > sql/item_sum.cc | 45 ++++++++-- > 6 files changed, 177 insertions(+), 56 deletions(-) > > diff --git a/include/my_tree.h b/include/my_tree.h > index f8be55f..f1916b9 100644 > --- a/include/my_tree.h > +++ b/include/my_tree.h > @@ -57,11 +57,14 @@ typedef struct st_tree_element { > } TREE_ELEMENT; > > #define ELEMENT_CHILD(element, offs) (*(TREE_ELEMENT**)((char*)element + offs)) > +#define R_ELEMENT_CHILD(element, offs) ((TREE_ELEMENT**)((char*)element + offs)) Please, #define ELEMENT_CHILD_PTR(element, offs) ((TREE_ELEMENT**)((char*)element + offs)) #define ELEMENT_CHILD(element, offs) (*ELEMENT_CHILD_PTR(element, offs)) > typedef struct st_tree { > TREE_ELEMENT *root,null_element; > TREE_ELEMENT **parents[MAX_TREE_HEIGHT]; > + TREE_ELEMENT *free_element; > uint offset_to_key,elements_in_tree,size_of_element; > + uint elements_limit, del_direction; > size_t memory_limit, allocated; > qsort_cmp2 compare; > void *custom_arg; > diff --git a/mysql-test/r/group_concat_big.result b/mysql-test/r/group_concat_big.result > new file mode 100644 > index 0000000..4de0ebb > --- /dev/null > +++ b/mysql-test/r/group_concat_big.result > @@ -0,0 +1,6 @@ > +SELECT GROUP_CONCAT( seq, seq, seq, seq, seq, seq, seq, seq ORDER BY > +2,1,3,4,6,5,8,7 ) AS cfield1 FROM seq_1_to_50000000; > +cfield1 > +11111111,22222222,33333333,44444444,55555555,66666666,77777777,88888888,99999999,1010101010101010,1111111111111111,1212121212121212,1313131313131313,1414141414141414,1515151515151515,1616161616161616,1717171717171717,1818181818181818,1919191919191919,2020202020202020,2121212121212121,2222222222222222,2323232323232323,2424242424242424,2525252525252525,2626262626262626,2727272727272727,2828282828282828,2929292929292929,3030303030303030,3131313131313131,3232323232323232,3333333333333333,3434343434343434,3535353535353535,3636363636363636,3737373737373737,3838383838383838,3939393939393939,4040404040404040,4141414141414141,4242424242424242,4343434343434343,4444444444444444,4545454545454545,4646464646464646,4747474747474747,4848484848484848,4949494949494949,5050505050505050,5151515151515151,5252525252525252,5353535353535353,5454545454545454,5555555555555555,5656565656565656,5757575757575757,5858585858585858,5959595959595959,6060606060606060,6161616161616161,6262626262626262,6363636 > 363636363,6464646464646464,65656565 > +Warnings: > +Warning 1260 Row 65 was cut by GROUP_CONCAT() 1. Same result without your patch? 2. I suppose all rows after 65 skipped completely, aren't they? > diff --git a/mysql-test/t/group_concat_big.result b/mysql-test/t/group_concat_big.result > new file mode 100644 > index 0000000..4de0ebb > --- /dev/null > +++ b/mysql-test/t/group_concat_big.result > @@ -0,0 +1,6 @@ > +SELECT GROUP_CONCAT( seq, seq, seq, seq, seq, seq, seq, seq ORDER BY > +2,1,3,4,6,5,8,7 ) AS cfield1 FROM seq_1_to_50000000; > +cfield1 > +11111111,22222222,33333333,44444444,55555555,66666666,77777777,88888888,99999999,1010101010101010,1111111111111111,1212121212121212,1313131313131313,1414141414141414,1515151515151515,1616161616161616,1717171717171717,1818181818181818,1919191919191919,2020202020202020,2121212121212121,2222222222222222,2323232323232323,2424242424242424,2525252525252525,2626262626262626,2727272727272727,2828282828282828,2929292929292929,3030303030303030,3131313131313131,3232323232323232,3333333333333333,3434343434343434,3535353535353535,3636363636363636,3737373737373737,3838383838383838,3939393939393939,4040404040404040,4141414141414141,4242424242424242,4343434343434343,4444444444444444,4545454545454545,4646464646464646,4747474747474747,4848484848484848,4949494949494949,5050505050505050,5151515151515151,5252525252525252,5353535353535353,5454545454545454,5555555555555555,5656565656565656,5757575757575757,5858585858585858,5959595959595959,6060606060606060,6161616161616161,6262626262626262,6363636 > 363636363,6464646464646464,65656565 > +Warnings: > +Warning 1260 Row 65 was cut by GROUP_CONCAT() Interesting. How did you manage to include the same file twice in a commit diff? > diff --git a/mysys/tree.c b/mysys/tree.c > index a9fc542..6c094d9 100644 > --- a/mysys/tree.c > +++ b/mysys/tree.c > @@ -157,6 +172,10 @@ static void free_tree(TREE *tree, myf free_flags) > free_root(&tree->mem_root, free_flags); > } > } > + if (tree->free_element && tree->with_delete && tree->free) > + (*tree->free)(tree->free_element, free_free, > + tree->custom_arg); "&& tree->with_delete" is wrong here > + tree->free_element= 0; > tree->root= &tree->null_element; > tree->elements_in_tree=0; > tree->allocated=0; > @@ -190,6 +209,42 @@ static void delete_tree_element(TREE *tree, TREE_ELEMENT *element) > } > function comment would be nice here > +void tree_exclude(TREE *tree, TREE_ELEMENT ***parent) > +{ > + int remove_colour; > + TREE_ELEMENT ***org_parent, *nod; > + TREE_ELEMENT *element= **parent; > @@ -202,7 +257,32 @@ TREE_ELEMENT *tree_insert(TREE *tree, void *key, uint key_size, > void* custom_arg) > { > int cmp; > - TREE_ELEMENT *element,***parent; > + TREE_ELEMENT *element, ***parent; > + > + if (tree->elements_limit && tree->elements_in_tree && no need for "&& tree->elements_in_tree" > + tree->elements_in_tree >= tree->elements_limit) > + { > + /* > + The limit reached so we should remove one. > + It is done on the very beginning because: > + 1) "parents" will be used > + 2) removing make incorrect search pass what does that mean? > + If we will not insert now, we leave freed element for future use > + */ > + DBUG_ASSERT(key_size == 0); why? > + > + parent= tree->parents; > + *parent = &tree->root; element= tree->root; > + while (element != &tree->null_element) > + { > + *++parent= R_ELEMENT_CHILD(element, tree->del_direction); > + element= ELEMENT_CHILD(element, tree->del_direction); > + } > + parent--; > + tree->free_element= **parent; > + tree_exclude(tree, parent); > + tree->elements_in_tree--; > + } > > parent= tree->parents; > *parent = &tree->root; element= tree->root; > diff --git a/sql/item_sum.cc b/sql/item_sum.cc > index 1cfee1a..8bcc2ce 100644 > --- a/sql/item_sum.cc > +++ b/sql/item_sum.cc > @@ -3390,7 +3390,7 @@ Item_func_group_concat::fix_fields(THD *thd, Item **ref) > fixed= 1; > return FALSE; > } > - > +struct tree_node_size {void *left,*right; uint32 f;}; why do you need that? > > bool Item_func_group_concat::setup(THD *thd) > { > @@ -3499,17 +3499,50 @@ bool Item_func_group_concat::setup(THD *thd) > > if (arg_count_order) > { > + uint min_rec_length= 1; //at least 1 byte per rec (coma) > + { > + List_iterator_fast<Item> li(all_fields); > + Item *item; > + while ((item= li++)) > + { > + switch (item->result_type()) > + { > + case STRING_RESULT: > + break; // could be empty string > + case DECIMAL_RESULT: > + case REAL_RESULT: > + min_rec_length+=2; > + break; > + case INT_RESULT: > + min_rec_length++; > + break; > + case TIME_RESULT: > + min_rec_length+=6; > + break; > + default: > + case ROW_RESULT: > + DBUG_ASSERT(0); > + break; > + } > + } > + } yeah... that'll work, of course, but for strings (most common case) you'll allocates tens or hundreds (if not thousands) times more memory than needed :( A hackish workaround could be to adjust tree->elements_limit (in Item_func_group_concat::add) after each insertion. But in this case it would be simpler to limit the tree by size (in bytes) and adjust tree size after each insertion. What do you think about it? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

[Maria-developers] mysql_get_timeout_value returning 0 always
by Gonzalo Diethelm 24 Sep '16

24 Sep '16

I am basing my work on the async_queries.c example. In my code, I am calling mysql_real_connect_start(), and the return value indicates that MySQL is waiting for READ and TIMEOUT. However, if I call mysql_get_timeout_value() (or the _ms() version) right after that, it returns 0. Is this expected? As far as I can see, this is causing the next call to mysql_real_connect_cont() (which happens immediately, because the timeout triggers right away) to return an error. This is on OS X 10.11.6 with the MariaDB client just installed via brew; mariadb_config --version says 5.5.1. This is the code I'm calling: status = mysql_real_connect_start(&ret, &mysql, h, u, p, d, 0, 0, 0); printf("MYSQL connect %d", status); if (status & MYSQL_WAIT_TIMEOUT) { unsigned int ms = mysql_get_timeout_value_ms(&mysql); printf(" and includes a timeout of %u ms", ms); } printf("\n"); That code prints: MYSQL connect 9 and includes a timeout of 0 ms I did find two ways to make the code work: 1. Not passing MYSQL_WAIT_TIMEOUT back to the mysql_real_connect_cont() (so it thinks a timeout has not happened at all). This allows all the queries to actually complete, but the code ends up polling heavily. 2. Forcing the return value of mysql_get_timeout_value_ms() to be > 0. This makes the code work beautifully, but of course I would rather follow MySQL's hint as to the timeout extension. I would appreciate any hints as to what I am missing. Thanks much in advance. -- Gonzalo Diethelm gonzalo.diethelm(a)gmail.com

2 4

Re: [Maria-developers] 8ffded0: MDEV-9312: storage engine not enforced during galera cluster replication
by Sergei Golubchik 24 Sep '16

24 Sep '16

Hi, Nirbhay! On Sep 24, Nirbhay Choubey wrote: > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index 60248f3..99b3311 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -3116,7 +3118,12 @@ void plugin_thdvar_init(THD *thd) > thd->variables.dynamic_variables_size= 0; > thd->variables.dynamic_variables_ptr= 0; > > - if (IF_WSREP((!WSREP(thd) || !thd->wsrep_applier),1)) > + /* > + The following initializations are deferred for some wsrep system threads > + created during startup as they could be created even before LOCK_plugin > + and plugins are initialized. > + */ > + if (IF_WSREP((plugins_are_initialized),1)) why do you need that? I think you can safely call plugin_thdvar_init() twice, so you don't really need to skip the first invocation. It's that principle that the one reponsible for the problem should pay for it, others shouldn't suffer :) In ha_maria:implicit_commit, it was Aria's hack, and that if() was added inside Aria. Here is purely wsrep problem, putting an if() on a common code path for all threads looks just wrong. > { > mysql_mutex_lock(&LOCK_plugin); > thd->variables.table_plugin= Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] Unique Blob Index for lower transaction isolation levels
by Sachin Setia 22 Sep '16

22 Sep '16

Hi Sergei, Actually I was thinking a different way of implementing this. in function ha_write_row , after it calls storage engine write, we can check for inserted values and see if it is inserted more than once. Of course, it can be I/O intensive (or it may not be , because data should be in buffer ). We have to do this only when there is multiple clients doing inserts in same table. But I am not sure if there is any way to find this. What do you think ? Regards sachin

1 0

Re: [Maria-developers] [Commits] 949e2ae: MDEV-10315 - Online ALTER TABLE may get stuck in tdc_remove_table
by Sergei Golubchik 21 Sep '16

21 Sep '16

Hi, Sergey! On Jul 01, Sergey Vojtovich wrote: > revision-id: 949e2aede909686053ab83ebf2741901be416a07 (mariadb-10.0.26-4-g949e2ae) > parent(s): 0fdb17e6c3f50ae22eb97b6363bcbd8b0cd9e040 > committer: Sergey Vojtovich > timestamp: 2016-07-01 13:57:18 +0400 > message: > > MDEV-10315 - Online ALTER TABLE may get stuck in tdc_remove_table > > There was race condition between online ALTER TABLE and statements performing > TABLE_SHARE release without marking it flushed (e.g. in case of table cache > overflow, SET @@global.table_open_cache, manager thread purgine table cache). > > The reason was missing mysql_cond_broadcast(). ok to push Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MDEV-10425 Assertion `collation.derivation == DERIVATION_IMPLICIT' failed in Item_func_conv_charset::fix_length_and_dec()
by Alexander Barkov 21 Sep '16

21 Sep '16

Hello Sergei, Please review. Thanks!

2 1

Re: [Maria-developers] [Commits] a288cb6: MDEV-8320 Allow index usage for DATE(datetime_column) = const.
by Sergey Petrunia 20 Sep '16

20 Sep '16

Hi Alexey, The patch doesn't have any testcase. Did you forget to add them? On Tue, Sep 20, 2016 at 01:22:19PM +0400, Alexey Botchkov wrote: > revision-id: a288cb698195b1e57abbb426f1cc9a804d65ff45 (mariadb-10.1.8-262-ga288cb6) > parent(s): cb575abf76be82553b9c1c12c9112cbc6f53a547 > committer: Alexey Botchkov > timestamp: 2016-09-20 13:19:08 +0400 > message: > > MDEV-8320 Allow index usage for DATE(datetime_column) = const. > > create_reverse_func() method added so functions can specify how > to unpack field argument out of it. > opt_arguments added to Item_bool_func2 so it can have different > arguments for the optimizer and the calcualtion itself. > > --- > sql/item.h | 8 +++++ > sql/item_cmpfunc.h | 52 ++++++++++++------------------- > sql/item_func.h | 5 +++ > sql/item_timefunc.cc | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > sql/item_timefunc.h | 49 ++++++++++++++++++++++++++++- > sql/opt_range.cc | 48 +++++++++++++++++++++++++++++ > sql/sql_select.cc | 70 ++++++++++++++++++++++++++++++++++++++---- > 7 files changed, 279 insertions(+), 40 deletions(-) > > diff --git a/sql/item.h b/sql/item.h > index 5b82548..200e2e0 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -1212,6 +1212,14 @@ class Item: public Value_source, > { > return; > } > + virtual bool add_extra_key_fields(THD *thd, > + JOIN *join, KEY_FIELD **key_fields, > + uint *and_level, > + table_map usable_tables, > + SARGABLE_PARAM **sargables) > + { > + return false; > + } > /* > Make a select tree for all keys in a condition or a condition part > @param param Context > 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. > + */ > + Item *opt_args[3]; > + uint opt_arg_count; > + > + /* > Build a SEL_TREE for a simple predicate > @param param PARAM from SQL_SELECT::test_quick_select > @param field field in the predicate > @@ -189,12 +197,12 @@ class Item_bool_func :public Item_int_func > KEY_PART *key_part, > Item_func::Functype type, Item *value); > public: > - Item_bool_func(THD *thd): Item_int_func(thd) {} > - Item_bool_func(THD *thd, Item *a): Item_int_func(thd, a) {} > - Item_bool_func(THD *thd, Item *a, Item *b): Item_int_func(thd, a, b) {} > - Item_bool_func(THD *thd, Item *a, Item *b, Item *c): Item_int_func(thd, a, b, c) {} > - Item_bool_func(THD *thd, List<Item> &list): Item_int_func(thd, list) { } > - Item_bool_func(THD *thd, Item_bool_func *item) :Item_int_func(thd, item) {} > + Item_bool_func(THD *thd): Item_int_func(thd), opt_arg_count(0) {} > + Item_bool_func(THD *thd, Item *a): Item_int_func(thd, a), opt_arg_count(0) {} > + Item_bool_func(THD *thd, Item *a, Item *b): Item_int_func(thd, a, b), opt_arg_count(0) {} > + Item_bool_func(THD *thd, Item *a, Item *b, Item *c): Item_int_func(thd, a, b, c), opt_arg_count(0) {} > + Item_bool_func(THD *thd, List<Item> &list): Item_int_func(thd, list), opt_arg_count(0) { } > + Item_bool_func(THD *thd, Item_bool_func *item) :Item_int_func(thd, item), opt_arg_count(0) {} > bool is_bool_type() { return true; } > virtual CHARSET_INFO *compare_collation() const { return NULL; } > void fix_length_and_dec() { decimals=0; max_length=1; } > @@ -436,33 +444,7 @@ class Item_bool_func2_with_rev :public Item_bool_func2 > Item_bool_func2_with_rev(THD *thd, Item *a, Item *b): > Item_bool_func2(thd, a, b) { } > virtual enum Functype rev_functype() const= 0; > - SEL_TREE *get_mm_tree(RANGE_OPT_PARAM *param, Item **cond_ptr) > - { > - DBUG_ENTER("Item_bool_func2_with_rev::get_mm_tree"); > - DBUG_ASSERT(arg_count == 2); > - SEL_TREE *ftree; > - /* > - Even if get_full_func_mm_tree_for_args(param, args[0], args[1]) will not > - return a range predicate it may still be possible to create one > - by reversing the order of the operands. Note that this only > - applies to predicates where both operands are fields. Example: A > - query of the form > - > - WHERE t1.a OP t2.b > - > - In this case, args[0] == t1.a and args[1] == t2.b. > - When creating range predicates for t2, > - get_full_func_mm_tree_for_args(param, args[0], args[1]) > - will return NULL because 'field' belongs to t1 and only > - predicates that applies to t2 are of interest. In this case a > - call to get_full_func_mm_tree_for_args() with reversed operands > - may succeed. > - */ > - if (!(ftree= get_full_func_mm_tree_for_args(param, args[0], args[1])) && > - !(ftree= get_full_func_mm_tree_for_args(param, args[1], args[0]))) > - ftree= Item_func::get_mm_tree(param, cond_ptr); > - DBUG_RETURN(ftree); > - } > + SEL_TREE *get_mm_tree(RANGE_OPT_PARAM *param, Item **cond_ptr); > }; > > > @@ -504,6 +486,10 @@ class Item_bool_rowready_func2 :public Item_bool_func2_with_rev > Item_bool_func2::cleanup(); > cmp.cleanup(); > } > + bool add_extra_key_fields(THD *thd, > + JOIN *join, KEY_FIELD **key_fields, > + uint *and_level, table_map usable_tables, > + SARGABLE_PARAM **sargables); > void add_key_fields(JOIN *join, KEY_FIELD **key_fields, > uint *and_level, table_map usable_tables, > SARGABLE_PARAM **sargables) > diff --git a/sql/item_func.h b/sql/item_func.h > index ca7c481..1f802db 100644 > --- a/sql/item_func.h > +++ b/sql/item_func.h > @@ -358,6 +358,11 @@ class Item_func :public Item_func_or_sum > - or replaced to an Item_int_with_ref > */ > bool setup_args_and_comparator(THD *thd, Arg_comparator *cmp); > + virtual bool create_reverse_func(enum Functype cmp_type, > + THD *thd, Item *r_arg, uint *a_cnt, Item** a) > + { > + return false; > + } > }; > > > 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) > +{ > + switch (cmp_type) > + { > + case GT_FUNC: > + case LE_FUNC: > + (*a_cnt)++; > + if (!(a[0]= new (thd->mem_root) Item_func_day_end(thd, r_arg)) || > + a[0]->fix_fields(thd, a+1)) > + return true; > + break; > + case LT_FUNC: > + case GE_FUNC: > + (*a_cnt)++; > + if (!(a[0]= new (thd->mem_root) Item_func_day_begin(thd, r_arg)) || > + a[0]->fix_fields(thd, a+1)) > + return true; > + break; > + case EQ_FUNC: > + (*a_cnt)+= 2; > + if (!(a[0]= new (thd->mem_root) Item_func_day_begin(thd, r_arg)) || > + a[0]->fix_fields(thd, a+1)) > + return true; > + if (!(a[1]= new (thd->mem_root) Item_func_day_end(thd, r_arg)) || > + a[1]->fix_fields(thd, a+2)) > + return true; > + default:; > + } > + return false; > +} > + > + > bool Item_datetime_typecast::get_date(MYSQL_TIME *ltime, ulonglong fuzzy_date) > { > fuzzy_date |= sql_mode_for_dates(current_thd); > @@ -3240,3 +3273,57 @@ bool Item_func_last_day::get_date(MYSQL_TIME *ltime, ulonglong fuzzy_date) > ltime->time_type= MYSQL_TIMESTAMP_DATE; > return (null_value= 0); > } > + > + > +bool Item_func_day_begin::get_date(MYSQL_TIME *res, ulonglong fuzzy_date) > +{ > + if (get_arg0_date(res, fuzzy_date)) > + return (null_value=1); > + > + res->second_part= res->second= res->minute= res->hour= 0; > + res->time_type= MYSQL_TIMESTAMP_DATETIME; > + > + return null_value= 0; > +} > + > + > +bool Item_func_day_end::get_date(MYSQL_TIME *res, ulonglong fuzzy_date) > +{ > + if (get_arg0_date(res, fuzzy_date)) > + return (null_value=1); > + > + res->hour= 23; > + res->second= res->minute= 59; > + res->second_part= 999999; > + res->time_type= MYSQL_TIMESTAMP_DATETIME; > + return null_value= 0; > +} > + > + > +bool Item_func_year_begin::get_date(MYSQL_TIME *res, ulonglong fuzzy_date) > +{ > + res->year= args[0]->val_int(); > + if ((null_value= args[0]->null_value || res->year >= 9999)) > + return 0; > + > + res->day= res->month= 1; > + res->second_part= res->second= res->minute= res->hour= 0; > + res->time_type= MYSQL_TIMESTAMP_DATETIME; > + return null_value= 0; > +} > + > + > +bool Item_func_year_end::get_date(MYSQL_TIME *res, ulonglong fuzzy_date) > +{ > + res->year= args[0]->val_int(); > + if ((null_value= args[0]->null_value || res->year >= 9999)) > + return 0; > + > + res->month= 12; > + res->day= 31; > + res->hour= 23; > + res->second= res->minute= 59; > + res->second_part= 999999; > + res->time_type= MYSQL_TIMESTAMP_DATETIME; > + return null_value= 0; > +} > diff --git a/sql/item_timefunc.h b/sql/item_timefunc.h > index a853c63..b4f64ef 100644 > --- a/sql/item_timefunc.h > +++ b/sql/item_timefunc.h > @@ -745,7 +745,7 @@ class Item_func_now_local :public Item_func_now > { > public: > Item_func_now_local(THD *thd, uint dec): Item_func_now(thd, dec) {} > - const char *func_name() const { return "now"; } > + const char *func_name() const { return "day_start"; } > virtual void store_now_in_TIME(THD *thd, MYSQL_TIME *now_time); > virtual enum Functype functype() const { return NOW_FUNC; } > Item *get_copy(THD *thd, MEM_ROOT *mem_root) > @@ -1074,6 +1074,8 @@ class Item_date_typecast :public Item_temporal_typecast > bool get_date(MYSQL_TIME *ltime, ulonglong fuzzy_date); > const char *cast_type() const { return "date"; } > enum_field_types field_type() const { return MYSQL_TYPE_DATE; } > + bool create_reverse_func(enum Functype cmp_type, > + THD *thd, Item *r_arg, uint *a_cnt, Item** a); > Item *get_copy(THD *thd, MEM_ROOT *mem_root) > { return get_item_copy<Item_date_typecast>(thd, mem_root, this); } > }; > @@ -1268,4 +1270,49 @@ class Item_func_last_day :public Item_datefunc > { return get_item_copy<Item_func_last_day>(thd, mem_root, this); } > }; > > + > +class Item_func_day_begin :public Item_datetimefunc > +{ > +public: > + Item_func_day_begin(THD *thd, Item *a): Item_datetimefunc(thd, a) {} > + const char *func_name() const { return "day_begin"; } > + bool get_date(MYSQL_TIME *res, ulonglong fuzzy_date); > + Item *get_copy(THD *thd, MEM_ROOT *mem_root) > + { return get_item_copy<Item_func_day_begin>(thd, mem_root, this); } > +}; > + > + > +class Item_func_day_end :public Item_datetimefunc > +{ > +public: > + Item_func_day_end(THD *thd, Item *a): Item_datetimefunc(thd, a) {} > + const char *func_name() const { return "day_end"; } > + bool get_date(MYSQL_TIME *res, ulonglong fuzzy_date); > + Item *get_copy(THD *thd, MEM_ROOT *mem_root) > + { return get_item_copy<Item_func_day_end>(thd, mem_root, this); } > +}; > + > + > +class Item_func_year_begin :public Item_datetimefunc > +{ > +public: > + Item_func_year_begin(THD *thd, Item *a): Item_datetimefunc(thd, a) {} > + const char *func_name() const { return "year_begin"; } > + bool get_date(MYSQL_TIME *res, ulonglong fuzzy_date); > + Item *get_copy(THD *thd, MEM_ROOT *mem_root) > + { return get_item_copy<Item_func_year_begin>(thd, mem_root, this); } > +}; > + > + > +class Item_func_year_end :public Item_datetimefunc > +{ > +public: > + Item_func_year_end(THD *thd, Item *a): Item_datetimefunc(thd, a) {} > + const char *func_name() const { return "year_end"; } > + bool get_date(MYSQL_TIME *res, ulonglong fuzzy_date); > + Item *get_copy(THD *thd, MEM_ROOT *mem_root) > + { return get_item_copy<Item_func_year_end>(thd, mem_root, this); } > +}; > + > + > #endif /* ITEM_TIMEFUNC_INCLUDED */ > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index 3ea9f4e..e533608 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -6998,6 +6998,54 @@ SEL_TREE *Item_bool_func::get_ne_mm_tree(RANGE_OPT_PARAM *param, > } > > > +SEL_TREE *Item_bool_func2_with_rev::get_mm_tree(RANGE_OPT_PARAM *param, Item **cond_ptr) > +{ > + DBUG_ENTER("Item_bool_func2_with_rev::get_mm_tree"); > + DBUG_ASSERT(arg_count == 2); > + SEL_TREE *ftree; > + /* > + Even if get_full_func_mm_tree_for_args(param, args[0], args[1]) will not > + return a range predicate it may still be possible to create one > + by reversing the order of the operands. Note that this only > + applies to predicates where both operands are fields. Example: A > + query of the form > + > + WHERE t1.a OP t2.b > + > + In this case, args[0] == t1.a and args[1] == t2.b. > + When creating range predicates for t2, > + get_full_func_mm_tree_for_args(param, args[0], args[1]) > + will return NULL because 'field' belongs to t1 and only > + predicates that applies to t2 are of interest. In this case a > + call to get_full_func_mm_tree_for_args() with reversed operands > + may succeed. > + */ > + if (opt_arg_count) > + { > + if (opt_arg_count == 2) > + { > + ftree= get_full_func_mm_tree_for_args(param, opt_args[0], opt_args[1]); > + } > + else if (opt_arg_count == 3) > + { > + Field *f= ((Item_field *) opt_args[0])->field; > + ftree= get_mm_parts(param, f, Item_func::GE_FUNC, opt_args[1]); > + if (ftree) > + { > + ftree= tree_and(param, ftree, > + get_mm_parts(param, f, > + Item_func::LE_FUNC, opt_args[2])); > + } > + } > + } > + if (!ftree && > + !(ftree= get_full_func_mm_tree_for_args(param, args[0], args[1])) && > + !(ftree= get_full_func_mm_tree_for_args(param, args[1], args[0]))) > + ftree= Item_func::get_mm_tree(param, cond_ptr); > + DBUG_RETURN(ftree); > +}; > + > + > SEL_TREE *Item_func_between::get_func_mm_tree(RANGE_OPT_PARAM *param, > Field *field, Item *value) > { > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index aa08420..51f6204 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -4833,6 +4833,30 @@ is_local_field (Item *field) > } > > > +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; > +} > + > + > +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; > +} > + > + > /* > In this and other functions, and_level is a number that is ever-growing > and is different for the contents of every AND or OR clause. For example, > @@ -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()) > + { > + opt_arg_count= 1; > + opt_args[0]= f; > + if (((Item_func *) args[0])->create_reverse_func( > + functype(), thd, args[1], &opt_arg_count, opt_args+1)) > + return true; > + } > + return false; > +} > + > + > void > Item_bool_func2::add_key_fields_optimize_op(JOIN *join, KEY_FIELD **key_fields, > uint *and_level, > @@ -5043,19 +5086,28 @@ Item_bool_func2::add_key_fields_optimize_op(JOIN *join, KEY_FIELD **key_fields, > SARGABLE_PARAM **sargables, > bool equal_func) > { > + Item_field *f; > /* If item is of type 'field op field/constant' add it to key_fields */ > - if (is_local_field(args[0])) > + if ((f= get_local_field(args[0]))) > { > - add_key_equal_fields(join, key_fields, *and_level, this, > - (Item_field*) args[0]->real_item(), equal_func, > + add_key_equal_fields(join, key_fields, *and_level, this, f, equal_func, > args + 1, 1, usable_tables, sargables); > } > - if (is_local_field(args[1])) > + else if ((f= get_local_field(args[1]))) > { > - add_key_equal_fields(join, key_fields, *and_level, this, > - (Item_field*) args[1]->real_item(), equal_func, > + add_key_equal_fields(join, key_fields, *and_level, this, f, equal_func, > args, 1, usable_tables, sargables); > } > + if (opt_arg_count == 2) > + { > + add_key_equal_fields(join, key_fields, *and_level, this, opt_args[0], > + equal_func, opt_args+1, 1, usable_tables, sargables); > + } > + else if (opt_arg_count == 3) > + { > + add_key_equal_fields(join, key_fields, *and_level, this, opt_args[0], > + false, opt_args+1, 2, usable_tables, sargables); > + } > } > > > @@ -5521,8 +5573,14 @@ update_ref_and_keys(THD *thd, DYNAMIC_ARRAY *keyuse,JOIN_TAB *join_tab, > if (cond) > { > KEY_FIELD *saved_field= field; > + > + if (cond->add_extra_key_fields(thd, join_tab->join, &end, &and_level, > + normal_tables, sargables)) > + DBUG_RETURN(TRUE); > + > cond->add_key_fields(join_tab->join, &end, &and_level, normal_tables, > sargables); > + > for (; field != end ; field++) > { > > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 8cd65cd: MDEV-9143 JSON_xxx functions.
by Sergei Golubchik 15 Sep '16

15 Sep '16

Hi, Alexey! On Sep 13, Alexey Botchkov wrote: > revision-id: 8cd65cd3e31aa6129573c6fbba9feb06c714e00c (mariadb-10.1.8-247-g8cd65cd) > parent(s): 76a0ed2e03c6ae1ff791534e742c812d5e83ba63 > committer: Alexey Botchkov > timestamp: 2016-09-13 15:48:03 +0400 > message: > > MDEV-9143 JSON_xxx functions. > > Library added to operate JSON format. > SQL functions to handle JSON data added: > required by SQL standard: > JSON_VALUE > JSON_QUERY > JSON_EXISTS > MySQL functions: > JSON_VALID > JSON_ARRAY > JSON_ARRAY_APPEND > JSON_CONTAINS_PATH > JSON_EXTRACT > JSON_OBJECT > JSON_QUOTE > JSON_MERGE > Some MySQL functions still missing, but no specific > difficulties to implement them too in same manner. > JSON_TABLE of the SQL standard is missing as it's not > clean how and whether we'd like to implement it. Good comment, thanks. > include/CMakeLists.txt | 1 + > include/json_lib.h | 338 ++++++++++ > mysql-test/r/func_json.result | 99 +++ > mysql-test/t/func_json.test | 44 ++ > sql/CMakeLists.txt | 2 +- > sql/item.h | 15 + > sql/item_create.cc | 351 ++++++++++ > sql/item_jsonfunc.cc | 898 ++++++++++++++++++++++++++ > sql/item_jsonfunc.h | 240 +++++++ > sql/item_xmlfunc.cc | 21 +- > sql/item_xmlfunc.h | 5 - > sql/sql_yacc.yy | 4 +- > strings/CMakeLists.txt | 2 +- > strings/ctype-ucs2.c | 19 +- > strings/json_lib.c | 1426 +++++++++++++++++++++++++++++++++++++++++ > 15 files changed, 3433 insertions(+), 32 deletions(-) Is the json library code the same as in the previous commit? Or should I look at it again? And *please* add unit tests for the json library. > diff --git a/include/json_lib.h b/include/json_lib.h > new file mode 100644 > index 0000000..592096f > --- /dev/null > +++ b/include/json_lib.h ... > + do > + { > + // The parser has read next piece of JSON > + // and set fields of j_eng structure accordingly. > + // So let's see what we have: Oops. Didn't notice that earlier, sorry. No C++ comments in C files, please, some older compilers don't like that. > diff --git a/mysql-test/r/func_json.result b/mysql-test/r/func_json.result > new file mode 100644 > index 0000000..1ebe0b2 > --- /dev/null > +++ b/mysql-test/r/func_json.result > @@ -0,0 +1,99 @@ > +select json_value('{"key1":[1,2,3]}', '$.key1'); > +json_value('{"key1":[1,2,3]}', '$.key1') > +NULL I'd expect an error here, but the standard explictly says: 5) If <JSON value error behavior> is not specified, then NULL ON ERROR is implicit. that is, returning NULL on errors is correct. Would be nice to have a comment about it in the code (around Item_func_json_value::val_str), otherwise someone might change this behavior to return an error. > +select json_value('{"key1": [1,2,3], "key1":123}', '$.key1'); > +json_value('{"key1": [1,2,3], "key1":123}', '$.key1') > +123 Eh... Okay, so you show the *last* key, right? SQL standard says it's implementation defined, so I suppose that's fine. > +select json_object("ki", 1, "mi", "ya"); > +json_object("ki", 1, "mi", "ya") > +{"ki": 1, "mi": "ya"} add tests for json constructing functions (json_object, json_array_append, etc) with key/values that need quoting or escaping. I mean, tests to show that json_object (for example) it more complex than concat(). > diff --git a/mysql-test/t/func_json.test b/mysql-test/t/func_json.test > new file mode 100644 > index 0000000..a5b84e0c > --- /dev/null > +++ b/mysql-test/t/func_json.test ... > + > +select json_merge('string', 123); how comes this test is not in the result file? > diff --git a/sql/item_jsonfunc.h b/sql/item_jsonfunc.h > new file mode 100644 > index 0000000..aff19ee > --- /dev/null > +++ b/sql/item_jsonfunc.h > +class json_path_with_flags > +{ > +public: > + json_path_t p; > + bool constant; > + bool parsed; > + json_path_step_t *cur_step; > + void set_constant_flag(bool s_constant) > + { > + constant= s_constant; > + parsed= FALSE; why set_constant_flag() resets 'parsed'? Better to set parsed=false in the constructor. > + } > +}; > + > diff --git a/sql/item_jsonfunc.cc b/sql/item_jsonfunc.cc > new file mode 100644 > index 0000000..5649989 > --- /dev/null > +++ b/sql/item_jsonfunc.cc ... > +/* > + Appends ASCII string to the String object taking it's charset in > + consideration. > +*/ > +static int st_append_ascii(String *s, const char *ascii, uint ascii_len) Why did you do that, instead of using String::append() ? I'd use String::append(), and if that would happen to be too slow, I'd implement String::append_ascii(), not something json-specific. ... > +/* > + Appends arbitrary String to the JSON string taking charsets in > + consideration. > +*/ > +static int st_append_escaped(String *s, const String *a) > +{ > + /* > + In the worst case one character from the 'a' string > + turns into '\uXXXX\uXXXX' which is 12. how comes? add an example, please. Like "for example, character x'1234' in the charset A becomes '\u1234\u5678' if JSON string is in the charset B" > + */ > + int str_len= a->length() * 12 * s->charset()->mbmaxlen / > + a->charset()->mbminlen; > + if (!s->reserve(str_len, 1024) && > + (str_len= > + json_escape(a->charset(), (uchar *) a->ptr(), (uchar *)a->end(), > + s->charset(), > + (uchar *) s->end(), (uchar *)s->end() + str_len)) > 0) > + { > + s->length(s->length() + str_len); > + return 0; > + } > + > + return a->length(); > +} ... > +longlong Item_func_json_exists::val_int() > +{ > + json_engine_t je; > + String *js= args[0]->val_str(&tmp_js); > + > + if (!path.parsed) > + { > + String *s_p= args[1]->val_str(&tmp_path); > + if (s_p && > + json_path_setup(&path.p, s_p->charset(), (const uchar *) s_p->ptr(), > + (const uchar *) s_p->ptr() + s_p->length())) 1. why wouldn't you do it in fix_length_and_dec for constant paths? 2. please add tests when path is not constant > + goto err_return; > + path.parsed= path.constant; > + } > + > + if ((null_value= args[0]->null_value || args[1]->null_value)) > + { > + null_value= 1; > + return 0; > + } > + > + null_value= 0; > + json_scan_start(&je, js->charset(),(const uchar *) js->ptr(), > + (const uchar *) js->ptr() + js->length()); > + > + path.cur_step= path.p.steps; > + if (json_find_value(&je, &path.p, &path.cur_step)) > + { > + if (je.s.error) > + goto err_return; > + return 0; > + } > + > + return 1; > + > +err_return: > + null_value= 1; > + return 0; > +} ... > +longlong Item_func_json_contains_path::val_int() > +{ > + String *js= args[0]->val_str(&tmp_js); > + json_engine_t je; > + uint n_arg; > + longlong result; > + > + if ((null_value= args[0]->null_value)) > + return 0; ... > + result= !mode_one; > + for (n_arg=2; n_arg < arg_count; n_arg++) > + { > + json_path_with_flags *c_path= paths + n_arg - 2; > + if (!c_path->parsed) > + { > + String *s_p= args[n_arg]->val_str(tmp_paths+(n_arg-2)); > + if (s_p && > + json_path_setup(&c_path->p,s_p->charset(),(const uchar *) s_p->ptr(), > + (const uchar *) s_p->ptr() + s_p->length())) > + goto error; > + c_path->parsed= TRUE; eh? not c_path->parsed= c_path->constant ? > + } > + > + json_scan_start(&je, js->charset(),(const uchar *) js->ptr(), > + (const uchar *) js->ptr() + js->length()); > + > + c_path->cur_step= c_path->p.steps; > + if (json_find_value(&je, &c_path->p, &c_path->cur_step)) > + { > + /* Path wasn't found. */ > + if (je.s.error) > + goto error; > + > + if (!mode_one) > + { > + result= 0; > + break; > + } > + } > + else if (mode_one) > + { > + result= 1; > + break; > + } > + } > + > + > + return result; > + > +error: > + null_value= 1; > + return 0; > +} Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0