List:Commits« Previous MessageNext Message »
From:Martin Hansson Date:June 1 2010 2:38pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (epotemkin:3200)
Bug#49771
View as plain text  
Hi Evgeny,
basically I am ready to approve this patch. And I also learned a bit 
along the way. But I have a few more questions, concerns and some 
nit-picking on the code.

Evgeny Potemkin skrev 2010-05-25 14.07:
> #At file:///work/bzrroot/49771-bug-next-mr-bugfixing/ based on
> revid:mattias.jonsson@stripped
> ...      since all datetime values has STRING result type MIN/MAX functions are
> asked
>       for a STRING value when the result is being sent to a client. The
>       Item_cache_datetime was designed to avoid conversions and get INT/STRING
>   
I think you actually mean

Item_cache_datetime was designed to avoid conversion by storing the 
value of the underlying expression.

Or did I not understand correctly?
>       ...
>       The equality propagation optimization is adjusted to take into account that
>       items which being compared as DATETIME can have different comparison
>       contexts.
>   
You have identified a general concept here: An expression can be 
interpreted as (cast to) different types depending on context. So I 
suggest a change of terminology:

::is_datetime_comparable -> ::is_contextually_compared_with (or 
something to that effect).

This way your enhancement of the type system does not depend directly on 
the datetime set of types. It will also be perfectly legitimate to add 
other such types (e.g. year-month interval or day-time interval) to a 
function with a name that doesn't contain 'datetime'.

>       The Item_cache_datetime now converts cached INT value to a correct STRING
>       DATETIME value by means of get_date & my_TIME_to_str functions.
>       The Arg_comparator::set_cmp_context_for_datetime helper function is added. 
>       It sets comparison context of items being compared as DATETIMEs to INT if
>       items will be compared as longlong.
>   
What happens if they will not? Can it happen?

> ...     representation is more precise than the string one).
>    */
>    virtual bool result_as_longlong() { return FALSE; }
> -  bool is_datetime();
> +  bool inline is_datetime()
>   
I think "inline bool" is a more common ordering. That makes grep'ing easier.
> ...
> +  bool inline is_datetime_comparable(Item *item)
> +  {
> +    bool datetime= FALSE;
> +    if (((datetime= is_datetime()) || cmp_context == STRING_RESULT) &&
> +        ((datetime|= item->is_datetime()) ||
> +          item->cmp_context == STRING_RESULT) &&
> +          datetime)
>   
Hmmm, this if statement is four line long, nests || within &&, contains 
assignments used as truth values twice, and it uses bit-masking on a 
boolean value. Isn't there a simpler way to express this?

>    virtual Field::geometry_type get_geometry_type() const
>      { return Field::GEOM_GEOMETRY; };
>    String *check_well_formed_result(String *str, bool send_error= 0);
> @@ -3193,6 +3219,7 @@ public:
>    virtual bool cache_value()= 0;
>    bool basic_const_item() const
>    { return test(example && example->basic_const_item());}
> +  virtual void clear() { null_value= 1; value_cached= FALSE; }
>   
Isn't this a bit inconsistent, first you use 1 to denote true, then 
FALSE to denote false.
>  ...
> +  void clear() { null_value= 1; value_cached= str_value_cached= FALSE; }
>  };
>   
Same here.

Otherwise, the patch looks good.

Best Regards

Martin
Thread
bzr commit into mysql-next-mr-bugfixing branch (epotemkin:3200)Bug#49771Evgeny Potemkin25 May
  • Re: bzr commit into mysql-next-mr-bugfixing branch (epotemkin:3200)Bug#49771Martin Hansson1 Jun
  • Re: bzr commit into mysql-next-mr-bugfixing branch (epotemkin:3200)Bug#49771Alexey Kopytov9 Jun
    • Re: bzr commit into mysql-next-mr-bugfixing branch (epotemkin:3200)Bug#49771Evgeny Potemkin12 Jul