Hi Alexander, I agree, your code is more clearer. I've added your additional tests and done the pull request.
Thank you very much. Jérôme. > -----Message d'origine----- > De : Alexander Barkov [mailto:b...@mariadb.org] > Envoyé : vendredi 13 octobre 2017 14:34 > À : jerome brauge; maria-developers > Objet : Re: MDEV-14013 > > Hello Jerome, > > Thank you very much for you contribution! > > > On 10/11/2017 02:32 PM, jerome brauge wrote: > > Hello Alexander, > > Here is the patch for MDEV-14013, can you review it ? > > The patch looks very good. I only found small things: > > > 1. LEX::make_text_literal_concat() > > thd->variables.character_set_client was used instread of > thd->variables.collation_connection > > > 2. LEX::make_text_literal_concat() was somewhat hard to read. > I propose to change the logic to do: > > if (item->type() == Item::NULL_ITEM) > { > } > > instead of > > if (item->type() != Item::NULL_ITEM || > !(thd->variables.sql_mode & MODE_EMPTY_STRING_IS_NULL)) > > I think it looks clearer this way. > Also, I added some DBUG_ASSERTs to make more sure that a non- > Item_string item is not erroneously cast to Item_string. > > > 3. Also, I found that it's easier to place the new methods into THD instead of > LEX. > > Please find a patch with changes 1,2,3 attached. > > Do you agree with these changes? > > > Also, can you please add some tests: > > - SHOW CREATE VIEW into "Test in a view" in empty_string_literal.result.diff > > - EXPLAIN EXTENDED for the affected grammar: > > EXPLAIN EXTENDED SELECT ''; > EXPLAIN EXTENDED SELECT _latin1''; > EXPLAIN EXTENDED SELECT N''; > EXPLAIN EXTENDED SELECT '' ''; > > > Thanks! > > > > > > Thanks. > > > > > >> -----Message d'origine----- > >> De : Alexander Barkov [mailto:b...@mariadb.org] Envoyé : mercredi 11 > >> octobre 2017 04:15 À : jerome brauge Objet : Re: Question about > >> MDEV-10574 > >> > >> Hello Jerome, > >> > >> > >> On 10/10/2017 04:23 PM, jerome brauge wrote: > >>> Hello Alexander, > >>> For sql_mode EMPTY_STRING_IS_NULL, do you think that it must cover > >>> call > >> of statement with use of user variable initialized with an empty string ? > >>> > >>> Ex : > >>> SET sql_mode=default; > >>> SET @param=''; > >>> > >>> SET sql_mode=EMPTY_STRING_IS_NULL; > >>> PREPARE test FROM 'select 1,?'; > >>> EXECUTE test USING @param; > >>> > >>> Or > >>> > >>> delimiter / > >>> CREATE OR REPLACE PROCEDURE p1(IN a VARCHAR(50)) BEGIN > >>> IF a is null THEN > >>> SELECT 'IS NULL'; > >>> ELSE > >>> select 'IS NOT NULL'; > >>> END IF; > >>> END; > >>> / > >>> delimiter ; > >>> SET sql_mode=default; > >>> SET @a=''; > >>> > >>> SET sql_mode=EMPTY_STRING_IS_NULL; > >>> EXECUTE IMMEDIATE 'CALL p1(?)' USING @a; CALL p1(@a); > >>> > >>> For me, it's a study case and we can forget it. > >> > >> I agree. Oracle doesn't have user variables. > >> > >>> > >>> Regards, > >>> Jérôme. > >>> > >>>> -----Message d'origine----- > >>>> De : Alexander Barkov [mailto:b...@mariadb.org] Envoyé : samedi 7 > >>>> octobre 2017 11:44 À : jerome brauge Objet : Re: Patch for > >>>> MDEV-10574 > >>>> > >>>> Hello Jerome, > >>>> > >>>> > >>>> On 10/06/2017 01:41 PM, jerome brauge wrote: > >>>>> Hello Alexander, > >>>>> > >>>>>> -----Message d'origine----- > >>>>>> De : Alexander Barkov [mailto:b...@mariadb.org] Envoyé : jeudi 5 > >>>>>> octobre 2017 21:26 À : jerome brauge Objet : Re: Patch for > >>>>>> MDEV-10574 > >>>>>> > >>>>>> Hello Jerome, > >>>>>> > >>>>>> > >>>>>> On 10/05/2017 12:45 PM, jerome brauge wrote: > >>>>>>> Hello Alexander, > >>>>>>> Have you gone ahead on your reflection about nulls and empty > >>>>>>> strings > >> ? > >>>>>> > >>>>>> Yes, I think so. > >>>>>> > >>>>>> One addition: Monty suggested to rename flags to: > >>>>>> - EMPTY_STRING_IS_NULL for literals > >>>>>> - EMPTY_STRING_IS_NULL_RESULTS for functions. > >>>>>> > >>>>>> I'm fine with EMPTY_STRING_IS_NULL for literals, but think that > >>>>>> EMPTY_STRING_IS_NULL_FUNCTIONS should be more clear than > >>>>>> EMPTY_STRING_IS_NULL_RESULTS. > >>>>>> Which name do you prefer for functions? > >>>>> > >>>>> I think like you. > >>>>> For me, EMPTY_STRING_IS_NULL_RESULTS sounds like including > >>>> RESULTSET > >>>>> of select (which is perhaps included in point 4) > >>>> > >>>> Okey, let's go with _FUNCTIONS then. > >>>> > >>>>> > >>>>>>> > >>>>>>> Can I > >>>>>>> - redo my pull request for substring_oracle function > >>>>>>> - make a patch for first new sql_mode > >>>>>> (MODE_EMPTY_AS_NULL_LITERALS) > >>>>>> > >>>>>> > >>>>>> Yes, please. Let's move forward. > >>>>>> > >>>>>> Let's push the patch for substr_oracle first. > >>>>>> Let's keep it return empty strings for the cases when the > >>>>>> substring_length parameters is less than 1. > >>>>>> It will start automatically returning NULL when we add the new > >>>>>> sql_mode for functions. > >>>>> > >>>>> Pull request is done. > >>>> > >>>> I have merged it. Thanks! > >>>> > >>>>> > >>>>>> Also, let's go ahead with the part for literals. > >>>>> > >>>>> Ok, I will make a separate patch. > >>>>> Many thanks. > >>>>> > >>>>>> > >>>>>> > >>>>>> I have created MDEVs. Please use them in the commit comment and > >>>>>> in the > >>>>>> tests: > >>>>>> > >>>>>> MDEV-14012 sql_mode=Oracle: substr(): treat position 0 as > >>>>>> position > >>>>>> 1 > >>>>>> MDEV-14013 sql_mode=EMPTY_STRING_IS_NULL > >>>>>> > >>>>>> > >>>>>> Thanks! > >>>>>> > >>>>>>> > >>>>>>> Regards, > >>>>>>> Jérôme. > >>>>>>> > >>>>>>> > >>>>>>>> -----Message d'origine----- > >>>>>>>> De : Alexander Barkov [mailto:b...@mariadb.org] Envoyé : > >>>>>>>> mercredi > >>>>>>>> 27 septembre 2017 08:50 À : jerome brauge Objet : Re: Patch for > >>>>>>>> MDEV-10574 > >>>>>>>> > >>>>>>>> Hello Jerome, > >>>>>>>> > >>>>>>>> I told to Monty yesterday. > >>>>>>>> > >>>>>>>> We were going to implement the following transformations: > >>>>>>>> > >>>>>>>>> We are considering to implement both of the following options > >>>>>>>>> (with a > >>>>>>>>> switch) > >>>>>>>>> > >>>>>>>>> Transform Insert > >>>>>>>>> - insert values ("") -> insert values (null) > >>>>>>>>> > >>>>>>>>> Transform Select > >>>>>>>>> > >>>>>>>>> where v=x => (v <> "" and V=X) > >>>>>>>>> where v is null => (v="" or v is null) > >>>>>>>>> > >>>>>>>> > >>>>>>>> See MDEV-10574 for details. > >>>>>>>> > >>>>>>>> Monty did not fully understand the idea of translating literals > >>>>>>>> and > >>>>>> functions > >>>>>>>> from empty strings to null. He asked if you can explain use > >>>>>>>> cases for these transformations. > >>>>>>>> > >>>>>>>> But we agreed that: > >>>>>>>> > >>>>>>>> 1. It's OK to have multiple NULL handling flags in sql_mode. > >>>>>>>> > >>>>>>>> 2. New flags will not be enabled in 10.3, neither in > >>>>>>>> sql_mode=DEFAULT, > >>>>>> nor in > >>>>>>>> sql_mode=ORACLE. So one will have to specify them explicitly: > >>>>>>>> set > >>>>>>>> > >>>>>> > >>>> > >> > sql_mode='ORACLE,MODE_null_transfrormation_flag1,MODE_null_transfor > >>>>>>>> mation_flag2'; > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> So I propose we introduce the following flags: > >>>>>>>> > >>>>>>>> > >>>>>>>> 1. MODE_EMPTY_AS_NULL_LITERALS - for literals and PS > >> parameters, > >>>> as > >>>>>> in > >>>>>>>> your patch. > >>>>>>>> > >>>>>>>> 2. MODE_EMPTY_AS_NULL_FUNCTIONS - for functions. > >>>>>>>> This should be done as a separate patch. > >>>>>>>> Note, your patch currently implements empty-to-null translation > >>>>>>>> only for Item_str_func descendants that use > >> make_empty_result(). > >>>>>>>> This patch should be extended to cover Item_str_func > descendants > >>>>>>>> which do not use make_empty_result(), as well as > >>>>>>>> all other Item_func_or_sum descendants > >>>>>>>> (which are not Item_str_func descendants). > >>>>>>>> > >>>>>>>> > >>>>>>>> 3. MODE_EMPTY_AS_NULL_UPDATES > >>>>>>>> > >>>>>>>> For INSERT and UPDATE transformation, as in MDEV: > >>>>>>>> > >>>>>>>> insert values ("") -> insert values (null) > >>>>>>>> update set a='' -> update set a=null; > >>>>>>>> > >>>>>>>> > >>>>>>>> 4. MODE_EMPTY_AS_NULL_PREDICATES > >>>>>>>> > >>>>>>>> For predicate transformation, as in MDEV: > >>>>>>>> > >>>>>>>> where v=x => (v <> "" and V=X) > >>>>>>>> where v is null => (v="" or v is null) > >>>>>>>> > >>>>>>>> > >>>>>>>> What do you think about this proposal with multiple flags? > >>>>>>>> Note, we can do N1 and N2 now, and add N3 and N4 and later. > >>>>>>>> > >>>>>>>> Also, can you please write a description why you need N1 and N2. > >>>>>>>> Monty thinks that N3 and N4 should cover most use cases. > >>>>>>>> So we need a good rationale for Monty and Sergei explaining why > >>>>>>>> we > >>>>>> adding > >>>>>>>> these flags N1 and N2. > >>>>>>>> > >>>>>>>> > >>>>>>>> Thanks! > >>>>>>>> > >>>>>>>> > >>>>>>>> On 09/26/2017 04:33 PM, jerome brauge wrote: > >>>>>>>>> Hi Alexander, > >>>>>>>>> I'm disappointed. > >>>>>>>>> Can we at least keep this part in sql_prepare : > >>>>>>>>> > >>>>>>>>> /* > >>>>>>>>> set_param_str_oracle : convert empty string to null value > >>>>>>>>> This allow to bind empty string in JDBC as a null value. > >>>>>>>>> Ex : > >>>>>>>>> String sel="select coalesce(?,'null param') from dual"; > >>>>>>>>> PreparedStatement ps_sel = m_cnx.prepareStatement(sel); > >>>>>>>>> ps_sel.setString(1, ""); > >>>>>>>>> ResultSet rs = ps_sel.executeQuery(); > >>>>>>>>> while (rs.next()) > >>>>>>>>> { > >>>>>>>>> System.out.println(rs.getString(1)); > >>>>>>>>> } > >>>>>>>>> Returns : 'null param' > >>>>>>>>> */ > >>>>>>>>> static void set_param_str_oracle(Item_param *param, uchar > >> **pos, > >>>>>> ulong > >>>>>>>>> len) { > >>>>>>>>> ulong length= get_param_length(pos, len); > >>>>>>>>> if (length == 0) > >>>>>>>>> param->set_null(); > >>>>>>>>> else > >>>>>>>>> { > >>>>>>>>> if (length > len) > >>>>>>>>> length= len; > >>>>>>>>> param->set_str((const char *) *pos, length); > >>>>>>>>> *pos+= length; > >>>>>>>>> } > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> Regards, > >>>>>>>>> Jérôme. > >>>>>>>>> > >>>>>>>>> > >>>>>>>>>> -----Message d'origine----- > >>>>>>>>>> De : Alexander Barkov [mailto:b...@mariadb.org] Envoyé : > mardi > >>>>>>>>>> 26 septembre 2017 13:34 À : jerome brauge Objet : Re: Patch > >>>>>>>>>> for > >>>>>>>>>> MDEV-10574 > >>>>>>>>>> > >>>>>>>>>> Hello Jérôme, > >>>>>>>>>> > >>>>>>>>>> I'm afraid we cannot accept this change at this point of time. > >>>>>>>>>> > >>>>>>>>>> The problem is that all around the code we assume that NULL > >>>>>>>>>> is not equal to ''. > >>>>>>>>>> > >>>>>>>>>> Changing '' to NULL only in the visible part of the iceberg > >>>>>>>>>> (such as in the parser and in > >>>>>>>>>> Item_str_func::make_empty_result()) > >>>>>>>>>> will likely introduce various anomalies in fundamental things > >>>>>>>>>> like joins execution, optimizer, equality propagation, etc. > >>>>>>>>>> > >>>>>>>>>> Sorry, we don't have resources to dive into this topic deeper > now. > >>>>>>>>>> Moreover, Oracle's documentation suggests not to reply on the > >>>>>>>>>> fact > >>>>>> that '' > >>>>>>>>>> and NULL are treated as same, as this can change in the future. > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> On 09/20/2017 03:52 PM, jerome brauge wrote: > >>>>>>>>>>> Hello Alexander, > >>>>>>>>>>> Here is a new version of this patch (minor optimizations) > >>>>>>>>>>> > >>>>>>>>>>> Regards, > >>>>>>>>>>> Jérôme. > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>>> -----Message d'origine----- De : Maria-developers > >>>>>>>>>>>> [mailto:maria-developers- > >>>>>>>>>>>> bounces+j.brauge=qualiac....@lists.launchpad.net] De la > >>>>>>>>>>>> bounces+part > >>>> de > >>>>>>>>>>>> bounces+jerome > >>>>>>>>>>>> brauge > >>>>>>>>>>>> Envoyé : mardi 19 septembre 2017 16:09 À : 'Alexander > Barkov' > >>>>>>>>>>>> Cc : MariaDB Developers > >>>>>>>>>>>> (maria-developers@lists.launchpad.net) > >>>>>>>>>>>> Objet : [Maria-developers] Patch for MDEV-10574 > >>>>>>>>>>>> > >>>>>>>>>>>> Alexander, > >>>>>>>>>>>> Here is the patch I was talking about. > >>>>>>>>>>>> It not address all aspects of MDEV-10574, especially, empty > >>>>>>>>>>>> strings in table column are not view as null. > >>>>>>>>>>>> But as in oracle mode, no empty string in columns should be > >>>>>>>>>>>> created, an fully oracle application will never fall in this > >>>>>>>>>>>> case. > >>>>>>>>>>>> > >>>>>>>>>>>> Regards, > >>>>>>>>>>>> Jérôme. > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>>> -----Message d'origine----- De : jerome brauge Envoyé : > >>>>>>>>>>>>> mardi 19 septembre 2017 14:13 À : 'Alexander > >> Barkov' > >>>>>>>>>>>>> Objet : RE: Oracle strings functions compatibility : > >>>>>>>>>>>>> substr > >>>>>>>>>>>>> > >>>>>>>>>>>>> Hello Alexander, > >>>>>>>>>>>>> Yes I'm aware. > >>>>>>>>>>>>> This point will be treated by my next patch which will > >>>>>>>>>>>>> also affect the others string functions (a part of MDEV- > 10574). > >>>>>>>>>>>>> I will refresh this new patch with the current development > >>>>>>>>>>>>> branch, and I submit to you. > >>>>>>>>>>>>> Regards. > >>>>>>>>>>>>> Jérôme. > >>>>>>>>>>>>> > >>>>>>>>>>>>>> -----Message d'origine----- De : Alexander Barkov > >>>>>>>>>>>>>> [mailto:b...@mariadb.org] Envoyé : > >>>> mardi > >>>>>> 19 > >>>>>>>>>>>>>> septembre 2017 13:16 À : jerome brauge Objet : Re: > Oracle > >>>>>>>>>>>>>> strings functions compatibility : substr > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Hi Jerome, > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> I noticed one more difference in SUBSTR: > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> SELECT SUBSTR('aaa',1,-1) FROM DUAL; > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> MariaDB returns an empty string. > >>>>>>>>>>>>>> Oracle returns NULL. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Here's Oracle's documentation: > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>> > >>>>>> > >> https://docs.oracle.com/cd/B19306_01/server.102/b14200/functions162. > >>>>>>>>>>>>>> ht > >>>>>>>>>>>>>> m > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> I think we should do this in the same patch, to avoid > >>>>>>>>>>>>>> behavior change in the future. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Would you like to try doing this additional change? > >>>>>>>>>>>>>> Or would you like me to make an incremental patch on > top > >> of > >>>>>> yours? > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Thanks! > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> On 09/19/2017 10:33 AM, jerome brauge wrote: > >>>>>>>>>>>>>>> Hello Alexander, > >>>>>>>>>>>>>>> It's done. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Do you have news from Sergei for MDEV-12874, MDEV- > >> 13417 > >>>>>> and > >>>>>>>>>>>> MDEV- > >>>>>>>>>>>>>> 13418 ? > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> I asked Sergei, awaiting for his answer. I'll let you know. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Thank you very much. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> -----Message d'origine----- De : Alexander Barkov > >>>>>>>>>>>>>>>> [mailto:b...@mariadb.org] Envoyé : lundi > >>>>>>>>>>>>>>>> 18 septembre 2017 18:12 À : jerome brauge Objet : Re: > >>>>>>>>>>>>>>>> Oracle strings functions compatibility : substr > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Hello Jerome, > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> On 09/18/2017 05:14 PM, jerome brauge wrote: > >>>>>>>>>>>>>>>>> Hello Alexander, > >>>>>>>>>>>>>>>>> I don't understand how I've missed such an obvious > >>>> solution ! > >>>>>>>>>>>>>>>>> Here is the new patch. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Thanks. Now it looks simpler. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Can you please do one small thing: > >>>>>>>>>>>>>>>> move get_position() from "public:" to "protected:". > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Ok to make a pull request after this change. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Thank you very much! > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Regards. > >>>>>>>>>>>>>>>>> Jérôme. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> -----Message d'origine----- De : Alexander Barkov > >>>>>>>>>>>>>>>>>> [mailto:b...@mariadb.org] Envoyé : > >>>>>> lundi > >>>>>>>>>>>>>>>>>> 18 septembre 2017 11:30 À : jerome brauge Objet : > Re: > >>>>>> Oracle > >>>>>>>>>>>>>>>>>> strings functions compatibility : substr > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> Hello Jerome, > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> On 08/17/2017 04:43 PM, jerome brauge wrote: > >>>>>>>>>>>>>>>>>>> Hello Alexander, > >>>>>>>>>>>>>>>>>>> Here is a patch for function SUBSTR in > >> sql_mode=oracle > >>>>>>>>>>>>>>>>>>> (If position is 0, > >>>>>>>>>>>>>>>>>> then it is treated as 1). > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> I'm thinking of how to get a smaller patch. > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> Wouldn't it be possible to introduce: > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> virtual longlong get_position(); > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> longlong Item_func_substr::get_position() { > >>>>>>>>>>>>>>>>>> return args[1]->val_int(); } > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> longlong Item_func_substr_oracle::get_position() > >>>>>>>>>>>>>>>>>> { > >>>>>>>>>>>>>>>>>> longlong pos= args[1]->val_int(); > >>>>>>>>>>>>>>>>>> return pos == 0 ? 1 : pos; } > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> and just replace all calls for args[1]->val_int() to > >>>>>> get_position() > >>>>>>>> ? > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> Thanks! > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> Regards, > >>>>>>>>>>>>>>>>>>> Jérôme. > >>>>>>>>>>>>>>>>>>> _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp