Commit ae33ebe5 authored by Alexander Barkov's avatar Alexander Barkov

MDEV-23525 Wrong result of MIN(time_expr) and MAX(time_expr) with GROUP BY

Problem:

When calculatung MIN() and MAX() in a query with GROUP BY, like this:

  SELECT MIN(time_expr), MAX(time_expr) FROM t1 GROUP BY i;

the code in Item_sum_min_max::update_field() erroneosly used
string format comparison, therefore '100:20:30' was considered as
smaller than '10:20:30'.

Fix:

1. Implementing low level "native" related methods in class Time:
     Time::Time(const Native &native)           - convert native to Time
     Time::to_native(Native *to, uint decimals) - convert Time to native

   The "native" binary representation for TIME is equal to
   the binary data format of Field_timef, which is used to
   store TIME when mysql56_temporal_format is ON (default).

2. Implementing Type_handler_time_common "native" related methods:

  Type_handler_time_common::cmp_native()
  Type_handler_time_common::Item_val_native_with_conversion()
  Type_handler_time_common::Item_val_native_with_conversion_result()
  Type_handler_time_common::Item_param_val_native()

3. Implementing missing "native representation" related methods
   in Field_time and Field_timef:

  Field_time::store_native()
  Field_time::val_native()
  Field_timef::store_native()
  Field_timef::val_native()

4. Implementing missing "native" related methods in all Items
   that can have the TIME data type:

  Item_timefunc::val_native()
  Item_name_const::val_native()
  Item_time_literal::val_native()
  Item_cache_time::val_native()
  Item_handled_func::val_native()

5. Marking Type_handler_time_common as "native ready".
   So now Item_sum_min_max::update_field() calculates
   values using min_max_update_native_field(),
   which uses native binary representation rather than string representation.

   Before this change, only the TIMESTAMP data type used native
   representation to calculate MIN() and MAX().

Benchmarks (see more details in MDEV):

  This change not only fixes the wrong result, but also
  makes a "SELECT .. MAX.. GROUP BY .." query faster:

  # TIME(0)
  CREATE TABLE t1 (id INT, time_col TIME) ENGINE=HEAP;
  INSERT INTO t1 VALUES (1,'10:10:10'); -- repeat this 1m times
  SELECT id, MAX(time_col) FROM t1 GROUP BY id;

  MySQL80: 0.159 sec
  10.3:    0.108 sec
  10.4:    0.094 sec (fixed)

  # TIME(6):
  CREATE TABLE t1 (id INT, time_col TIME(6)) ENGINE=HEAP;
  INSERT INTO t1 VALUES (1,'10:10:10.999999'); -- repeat this 1m times
  SELECT id, MAX(time_col) FROM t1 GROUP BY id;

  My80: 0.154
  10.3: 0.135
  10.4: 0.093 (fixed)
parent aa6cb7ed
......@@ -2286,5 +2286,127 @@ SELECT '1972-11-06 16:58:58' < TIME'20:31:05';
'1972-11-06 16:58:58' < TIME'20:31:05'
1
#
# MDEV-23525 Wrong result of MIN(time_expr) and MAX(time_expr) with GROUP BY
#
SET timestamp=UNIX_TIMESTAMP('2020-08-21 18:19:20');
CREATE PROCEDURE p1()
BEGIN
SELECT MIN(t), MAX(t) FROM t1;
SELECT i, MIN(t), MAX(t) FROM t1 GROUP BY i;
SELECT i, MIN(COALESCE(t)), MAX(COALESCE(t)) FROM t1 GROUP BY i;
SELECT i, MIN(t+INTERVAL 1 SECOND), MAX(t+INTERVAL 1 SECOND) FROM t1 GROUP BY i;
SELECT i, MIN(TIME'10:20:30'+INTERVAL 1 SECOND) FROM t1 GROUP BY i;
SELECT i, MIN(CURRENT_TIME), MAX(CURRENT_TIME) FROM t1 GROUP BY i;
SELECT
i,
MIN((SELECT MAX(CURRENT_TIME) FROM t1)),
MAX((SELECT MAX(CURRENT_TIME) FROM t1))
FROM t1 GROUP BY i;
SELECT
i,
MIN(NAME_CONST('name',TIME'10:20:30')),
MAX(NAME_CONST('name',TIME'10:20:30'))
FROM t1 GROUP BY i;
EXECUTE IMMEDIATE "SELECT i, MIN(?),MAX(?) FROM t1 GROUP BY i"
USING TIME'10:20:30', TIME'10:20:30';
END;
$$
CREATE TABLE t1 (i INT, t TIME);
INSERT INTO t1 VALUES (1,'10:20:30');
INSERT INTO t1 VALUES (1,'100:20:20');
CALL p1;
MIN(t) MAX(t)
10:20:30 100:20:20
i MIN(t) MAX(t)
1 10:20:30 100:20:20
i MIN(COALESCE(t)) MAX(COALESCE(t))
1 10:20:30 100:20:20
i MIN(t+INTERVAL 1 SECOND) MAX(t+INTERVAL 1 SECOND)
1 10:20:31 100:20:21
i MIN(TIME'10:20:30'+INTERVAL 1 SECOND)
1 10:20:31
i MIN(CURRENT_TIME) MAX(CURRENT_TIME)
1 18:19:20 18:19:20
i MIN((SELECT MAX(CURRENT_TIME) FROM t1)) MAX((SELECT MAX(CURRENT_TIME) FROM t1))
1 18:19:20 18:19:20
i MIN(NAME_CONST('name',TIME'10:20:30')) MAX(NAME_CONST('name',TIME'10:20:30'))
1 10:20:30 10:20:30
i MIN(?) MAX(?)
1 10:20:30 10:20:30
DROP TABLE t1;
CREATE TABLE t1 (i INT, t TIME(3));
INSERT INTO t1 VALUES (1,'10:20:30.123');
INSERT INTO t1 VALUES (1,'100:20:20.123');
CALL p1;
MIN(t) MAX(t)
10:20:30.123 100:20:20.123
i MIN(t) MAX(t)
1 10:20:30.123 100:20:20.123
i MIN(COALESCE(t)) MAX(COALESCE(t))
1 10:20:30.123 100:20:20.123
i MIN(t+INTERVAL 1 SECOND) MAX(t+INTERVAL 1 SECOND)
1 10:20:31.123 100:20:21.123
i MIN(TIME'10:20:30'+INTERVAL 1 SECOND)
1 10:20:31
i MIN(CURRENT_TIME) MAX(CURRENT_TIME)
1 18:19:20 18:19:20
i MIN((SELECT MAX(CURRENT_TIME) FROM t1)) MAX((SELECT MAX(CURRENT_TIME) FROM t1))
1 18:19:20 18:19:20
i MIN(NAME_CONST('name',TIME'10:20:30')) MAX(NAME_CONST('name',TIME'10:20:30'))
1 10:20:30 10:20:30
i MIN(?) MAX(?)
1 10:20:30 10:20:30
DROP TABLE t1;
CREATE TABLE t1 (i INT, t TIME(6));
INSERT INTO t1 VALUES (1,'10:20:30.123456');
INSERT INTO t1 VALUES (1,'100:20:20.123456');
CALL p1;
MIN(t) MAX(t)
10:20:30.123456 100:20:20.123456
i MIN(t) MAX(t)
1 10:20:30.123456 100:20:20.123456
i MIN(COALESCE(t)) MAX(COALESCE(t))
1 10:20:30.123456 100:20:20.123456
i MIN(t+INTERVAL 1 SECOND) MAX(t+INTERVAL 1 SECOND)
1 10:20:31.123456 100:20:21.123456
i MIN(TIME'10:20:30'+INTERVAL 1 SECOND)
1 10:20:31
i MIN(CURRENT_TIME) MAX(CURRENT_TIME)
1 18:19:20 18:19:20
i MIN((SELECT MAX(CURRENT_TIME) FROM t1)) MAX((SELECT MAX(CURRENT_TIME) FROM t1))
1 18:19:20 18:19:20
i MIN(NAME_CONST('name',TIME'10:20:30')) MAX(NAME_CONST('name',TIME'10:20:30'))
1 10:20:30 10:20:30
i MIN(?) MAX(?)
1 10:20:30 10:20:30
DROP TABLE t1;
SET @@global.mysql56_temporal_format=false;
CREATE TABLE t1 (i INT, t TIME(6));
INSERT INTO t1 VALUES (1,'10:20:30.123456');
INSERT INTO t1 VALUES (1,'100:20:20.123456');
CALL p1;
MIN(t) MAX(t)
10:20:30.123456 100:20:20.123456
i MIN(t) MAX(t)
1 10:20:30.123456 100:20:20.123456
i MIN(COALESCE(t)) MAX(COALESCE(t))
1 10:20:30.123456 100:20:20.123456
i MIN(t+INTERVAL 1 SECOND) MAX(t+INTERVAL 1 SECOND)
1 10:20:31.123456 100:20:21.123456
i MIN(TIME'10:20:30'+INTERVAL 1 SECOND)
1 10:20:31
i MIN(CURRENT_TIME) MAX(CURRENT_TIME)
1 18:19:20 18:19:20
i MIN((SELECT MAX(CURRENT_TIME) FROM t1)) MAX((SELECT MAX(CURRENT_TIME) FROM t1))
1 18:19:20 18:19:20
i MIN(NAME_CONST('name',TIME'10:20:30')) MAX(NAME_CONST('name',TIME'10:20:30'))
1 10:20:30 10:20:30
i MIN(?) MAX(?)
1 10:20:30 10:20:30
DROP TABLE t1;
SET @@global.mysql56_temporal_format=default;
DROP PROCEDURE p1;
SET timestamp=DEFAULT;
#
# End of 10.4 tests
#
......@@ -1490,6 +1490,66 @@ DROP TABLE t1;
SELECT '1972-11-06 16:58:58' < TIME'20:31:05';
--echo #
--echo # MDEV-23525 Wrong result of MIN(time_expr) and MAX(time_expr) with GROUP BY
--echo #
SET timestamp=UNIX_TIMESTAMP('2020-08-21 18:19:20');
DELIMITER $$;
CREATE PROCEDURE p1()
BEGIN
SELECT MIN(t), MAX(t) FROM t1;
SELECT i, MIN(t), MAX(t) FROM t1 GROUP BY i;
SELECT i, MIN(COALESCE(t)), MAX(COALESCE(t)) FROM t1 GROUP BY i;
SELECT i, MIN(t+INTERVAL 1 SECOND), MAX(t+INTERVAL 1 SECOND) FROM t1 GROUP BY i;
SELECT i, MIN(TIME'10:20:30'+INTERVAL 1 SECOND) FROM t1 GROUP BY i;
SELECT i, MIN(CURRENT_TIME), MAX(CURRENT_TIME) FROM t1 GROUP BY i;
SELECT
i,
MIN((SELECT MAX(CURRENT_TIME) FROM t1)),
MAX((SELECT MAX(CURRENT_TIME) FROM t1))
FROM t1 GROUP BY i;
SELECT
i,
MIN(NAME_CONST('name',TIME'10:20:30')),
MAX(NAME_CONST('name',TIME'10:20:30'))
FROM t1 GROUP BY i;
EXECUTE IMMEDIATE "SELECT i, MIN(?),MAX(?) FROM t1 GROUP BY i"
USING TIME'10:20:30', TIME'10:20:30';
END;
$$
DELIMITER ;$$
CREATE TABLE t1 (i INT, t TIME);
INSERT INTO t1 VALUES (1,'10:20:30');
INSERT INTO t1 VALUES (1,'100:20:20');
CALL p1;
DROP TABLE t1;
CREATE TABLE t1 (i INT, t TIME(3));
INSERT INTO t1 VALUES (1,'10:20:30.123');
INSERT INTO t1 VALUES (1,'100:20:20.123');
CALL p1;
DROP TABLE t1;
CREATE TABLE t1 (i INT, t TIME(6));
INSERT INTO t1 VALUES (1,'10:20:30.123456');
INSERT INTO t1 VALUES (1,'100:20:20.123456');
CALL p1;
DROP TABLE t1;
SET @@global.mysql56_temporal_format=false;
CREATE TABLE t1 (i INT, t TIME(6));
INSERT INTO t1 VALUES (1,'10:20:30.123456');
INSERT INTO t1 VALUES (1,'100:20:20.123456');
CALL p1;
DROP TABLE t1;
SET @@global.mysql56_temporal_format=default;
DROP PROCEDURE p1;
SET timestamp=DEFAULT;
--echo #
--echo # End of 10.4 tests
--echo #
......@@ -6011,6 +6011,24 @@ bool Field_time::get_date(MYSQL_TIME *ltime, date_mode_t fuzzydate)
}
int Field_time::store_native(const Native &value)
{
Time t(value);
DBUG_ASSERT(t.is_valid_time());
store_TIME(t);
return 0;
}
bool Field_time::val_native(Native *to)
{
MYSQL_TIME ltime;
get_date(&ltime, date_mode_t(0));
int warn;
return Time(&warn, &ltime, 0).to_native(to, decimals());
}
bool Field_time::send_binary(Protocol *protocol)
{
MYSQL_TIME ltime;
......@@ -6254,6 +6272,22 @@ bool Field_timef::get_date(MYSQL_TIME *ltime, date_mode_t fuzzydate)
return false;
}
int Field_timef::store_native(const Native &value)
{
DBUG_ASSERT(value.length() == my_time_binary_length(dec));
DBUG_ASSERT(Time(value).is_valid_time());
memcpy(ptr, value.ptr(), value.length());
return 0;
}
bool Field_timef::val_native(Native *to)
{
uint32 binlen= my_time_binary_length(dec);
return to->copy((const char*) ptr, binlen);
}
/****************************************************************************
** year type
** Save in a byte the year 0, 1901->2155
......
......@@ -3213,6 +3213,8 @@ class Field_time :public Field_temporal {
decimals() == from->decimals();
}
sql_mode_t conversion_depends_on_sql_mode(THD *, Item *) const;
int store_native(const Native &value);
bool val_native(Native *to);
int store_time_dec(const MYSQL_TIME *ltime, uint dec);
int store(const char *to,size_t length,CHARSET_INFO *charset);
int store(double nr);
......@@ -3334,6 +3336,8 @@ class Field_timef :public Field_time_with_dec {
}
int reset();
bool get_date(MYSQL_TIME *ltime, date_mode_t fuzzydate);
int store_native(const Native &value);
bool val_native(Native *to);
uint size_of() const { return sizeof(*this); }
};
......
......@@ -2011,6 +2011,11 @@ bool Item_name_const::get_date(THD *thd, MYSQL_TIME *ltime, date_mode_t fuzzydat
return rc;
}
bool Item_name_const::val_native(THD *thd, Native *to)
{
return val_native_from_item(thd, value_item, to);
}
bool Item_name_const::is_null()
{
return value_item->is_null();
......
......@@ -1311,16 +1311,13 @@ class Item: public Value_source,
{
/*
The default implementation for the Items that do not need native format:
- Item_basic_value
- Item_basic_value (default implementation)
- Item_copy
- Item_exists_subselect
- Item_sum_field
- Item_sum_or_func (default implementation)
- Item_proc
- Item_type_holder (as val_xxx() are never called for it);
- TODO: Item_name_const will need val_native() in the future,
when we add this syntax:
TIMESTAMP WITH LOCAL TIMEZONE'2001-01-01 00:00:00'
These hybrid Item types override val_native():
- Item_field
......@@ -1331,6 +1328,8 @@ class Item: public Value_source,
- Item_direct_ref
- Item_direct_view_ref
- Item_ref_null_helper
- Item_name_const
- Item_time_literal
- Item_sum_or_func
Note, these hybrid type Item_sum_or_func descendants
override the default implementation:
......@@ -3139,6 +3138,7 @@ class Item_name_const : public Item_fixed_hybrid
String *val_str(String *sp);
my_decimal *val_decimal(my_decimal *);
bool get_date(THD *thd, MYSQL_TIME *ltime, date_mode_t fuzzydate);
bool val_native(THD *thd, Native *to);
bool is_null();
virtual void print(String *str, enum_query_type query_type);
......@@ -4897,6 +4897,10 @@ class Item_time_literal: public Item_temporal_literal
String *val_str(String *to) { return Time(this).to_string(to, decimals); }
my_decimal *val_decimal(my_decimal *to) { return Time(this).to_decimal(to); }
bool get_date(THD *thd, MYSQL_TIME *res, date_mode_t fuzzydate);
bool val_native(THD *thd, Native *to)
{
return Time(thd, this).to_native(to, decimals);
}
Item *get_copy(THD *thd)
{ return get_item_copy<Item_time_literal>(thd, this); }
};
......@@ -6872,6 +6876,10 @@ class Item_cache_time: public Item_cache_temporal
{
return has_value() ? Time(this).to_decimal(to) : NULL;
}
bool val_native(THD *thd, Native *to)
{
return has_value() ? Time(thd, this).to_native(to, decimals) : true;
}
};
......
......@@ -497,6 +497,12 @@ class Item_handled_func: public Item_func
virtual longlong val_int(Item_handled_func *) const= 0;
virtual my_decimal *val_decimal(Item_handled_func *, my_decimal *) const= 0;
virtual bool get_date(THD *thd, Item_handled_func *, MYSQL_TIME *, date_mode_t fuzzydate) const= 0;
virtual bool val_native(THD *thd, Item_handled_func *, Native *to) const
{
DBUG_ASSERT(0);
to->length(0);
return true;
}
virtual const Type_handler *return_type_handler() const= 0;
virtual bool fix_length_and_dec(Item_handled_func *) const= 0;
};
......@@ -600,6 +606,10 @@ class Item_handled_func: public Item_func
{
return Time(item).to_string(to, item->decimals);
}
bool val_native(THD *thd, Item_handled_func *item, Native *to) const
{
return Time(thd, item).to_native(to, item->decimals);
}
};
......@@ -668,6 +678,10 @@ class Item_handled_func: public Item_func
{
return m_func_handler->get_date(thd, this, to, fuzzydate);
}
bool val_native(THD *thd, Native *to)
{
return m_func_handler->val_native(thd, this, to);
}
};
......
......@@ -598,6 +598,10 @@ class Item_timefunc :public Item_func
double val_real() { return Time(this).to_double(); }
String *val_str(String *to) { return Time(this).to_string(to, decimals); }
my_decimal *val_decimal(my_decimal *to) { return Time(this).to_decimal(to); }
bool val_native(THD *thd, Native *to)
{
return Time(thd, this).to_native(to, decimals);
}
};
......
......@@ -628,6 +628,23 @@ uint Interval_DDhhmmssff::fsp(THD *thd, Item *item)
}
bool Time::to_native(Native *to, uint decimals) const
{
if (!is_valid_time())
{
to->length(0);
return true;
}
uint len= my_time_binary_length(decimals);
if (to->reserve(len))
return true;
longlong tmp= TIME_to_longlong_time_packed(get_mysql_time());
my_time_packed_to_binary(tmp, (uchar*) to->ptr(), decimals);
to->length(len);
return false;
}
void Time::make_from_item(THD *thd, int *warn, Item *item, const Options opt)
{
*warn= 0;
......@@ -812,6 +829,28 @@ void Time::make_from_time(int *warn, const MYSQL_TIME *from)
}
uint Time::binary_length_to_precision(uint length)
{
switch (length) {
case 3: return 0;
case 4: return 2;
case 5: return 4;
case 6: return 6;
}
DBUG_ASSERT(0);
return 0;
}
Time::Time(const Native &native)
{
uint dec= binary_length_to_precision(native.length());
longlong tmp= my_time_packed_from_binary((const uchar *) native.ptr(), dec);
TIME_from_longlong_time_packed(this, tmp);
DBUG_ASSERT(is_valid_time());
}
Time::Time(int *warn, const MYSQL_TIME *from, long curdays)
{
switch (from->time_type) {
......@@ -1456,6 +1495,13 @@ Type_handler_timestamp_common::type_handler_for_native_format() const
}
const Type_handler *
Type_handler_time_common::type_handler_for_native_format() const
{
return &type_handler_time2;
}
/***************************************************************************/
const Type_handler *Type_handler_typelib::type_handler_for_item_field() const
......@@ -8391,6 +8437,51 @@ Type_handler_time_common::create_literal_item(THD *thd,
}
bool
Type_handler_time_common::Item_val_native_with_conversion(THD *thd,
Item *item,
Native *to) const
{
if (item->type_handler()->type_handler_for_native_format() ==
&type_handler_time2)
return item->val_native(thd, to);
return Time(thd, item).to_native(to, item->time_precision(thd));
}
bool
Type_handler_time_common::Item_val_native_with_conversion_result(THD *thd,
Item *item,
Native *to)
const
{
if (item->type_handler()->type_handler_for_native_format() ==
&type_handler_time2)
return item->val_native_result(thd, to);
MYSQL_TIME ltime;
if (item->get_date_result(thd, &ltime, Time::Options(thd)))
return true;
int warn;
return Time(&warn, &ltime, 0).to_native(to, item->time_precision(thd));
}
int Type_handler_time_common::cmp_native(const Native &a,
const Native &b) const
{
// Optimize a simple case: equal fractional precision:
if (a.length() == b.length())
return memcmp(a.ptr(), b.ptr(), a.length());
longlong lla= Time(a).to_packed();
longlong llb= Time(b).to_packed();
if (lla < llb)
return -1;
if (lla> llb)
return 1;
return 0;
}
bool Type_handler_timestamp_common::TIME_to_native(THD *thd,
const MYSQL_TIME *ltime,
Native *to,
......@@ -8501,6 +8592,15 @@ Type_handler_timestamp_common::Item_param_val_native(THD *thd,
}
bool
Type_handler_time_common::Item_param_val_native(THD *thd,
Item_param *item,
Native *to) const
{
return Time(thd, item).to_native(to, item->decimals);
}
LEX_CSTRING Charset::collation_specific_name() const
{
/*
......
......@@ -1308,6 +1308,7 @@ class Schema;
*/
class Time: public Temporal
{
static uint binary_length_to_precision(uint length);
public:
enum datetime_to_time_mode_t
{
......@@ -1540,6 +1541,7 @@ class Time: public Temporal
*/
Time(int *warn, bool neg, ulonglong hour, uint minute, const Sec6 &second);
Time() { time_type= MYSQL_TIMESTAMP_NONE; }
Time(const Native &native);
Time(Item *item)
:Time(current_thd, item)
{ }
......@@ -1695,6 +1697,7 @@ class Time: public Temporal
return !is_valid_time() ? 0 :
Temporal::to_double(neg, TIME_to_ulonglong_time(this), second_part);
}
bool to_native(Native *to, uint decimals) const;
String *to_string(String *str, uint dec) const
{
if (!is_valid_time())
......@@ -5324,6 +5327,12 @@ class Type_handler_time_common: public Type_handler_temporal_result
{
return MYSQL_TIMESTAMP_TIME;
}
bool is_val_native_ready() const { return true; }
const Type_handler *type_handler_for_native_format() const;
int cmp_native(const Native &a, const Native &b) const;
bool Item_val_native_with_conversion(THD *thd, Item *, Native *to) const;
bool Item_val_native_with_conversion_result(THD *thd, Item *, Native *to) const;
bool Item_param_val_native(THD *thd, Item_param *item, Native *to) const;
Item_literal *create_literal_item(THD *thd, const char *str, size_t length,
CHARSET_INFO *cs, bool send_error) const;
Item *create_typecast_item(THD *thd, Item *item,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment