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
