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:bar@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:bar@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:bar@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:bar@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:bar@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:bar@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.com@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:bar@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:bar@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:bar@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. >>>>>>>>>>>>>>