3504 Jorgen Loland 2011-12-20
BUG#13430588: WRONG RESULT FROM IMPLICITLY GROUPED QUERY
WITH CONST TABLE AND NO MATCHING ROWS
BUG#13422961: WRONG RESULTS FROM SELECT WITH AGGREGATES
AND IMPLICIT GROUPING + MYISAM OR MEM
BUG#11760517: 52935: MIN/MAX FAILS TO EVALUATE HAVING
CONDITION, RETURNS INCORRECT NULL RESULT
When implicitly grouped queries do not have any rows matching
the join predicates, a row based on NULL values is returned
(if not filtered out by the HAVING clause). This means that
(a) non-aggregated fields should have the value NULL and (b)
aggregated fields should be calculated based on NULL.
For (a), this is achieved by marking all tables as containing
only NULL values by calling mark_as_null_row(). For (b),
this is achieved by calling Item::no_rows_in_result().
There were multiple bugs in this area:
1) In locations where mark_as_null_row() was called, const
tables were ignored. This has been fixed by storing the
info mark_as_null_row() modifies for const tables, call
mark_as_null_row() for all tables including const tables,
send the result and finally restore const table info.
Note that save/restore is only required for subqueries
since the value will not be reused otherwise. This was
the case for end_send_group() and end_write_group()
2) Item::no_rows_in_result() was not called everywhere where
aggregate items needed to be calculated on NULL values. This
was the case for do_select().
3) mark_as_null_row() was not called everywhere non-aggregate
items needed to be set to NULL. This was the case for
do_select() and end_write_group().
4) the having clause was evaluated after (a) but before (b).
This was the case in return_zero_rows()
Additional cleanup:
* mark_as_null_row() no longer sets the NULL bits, only
table->null_row and table->status
* Field::is_null() now checks table->null_row before checking
the NULL bits.
@ mysql-test/r/group_by.result
Add tests for 13422961 and 13430588
@ mysql-test/r/having.result
Added test for 11760517
@ mysql-test/t/group_by.test
Add tests for 13422961 and 13430588
@ mysql-test/t/having.test
Added test for 11760517
@ sql/field.h
Field::is_null() now checks table->null_row before checking
the NULL bits.
@ sql/item_sum.cc
Minor doc update
@ sql/sql_executor.cc
Added save_const_null_info() and restore_const_null_info().
@ sql/table.h
mark_as_null_row() no longer sets the NULL bits of the buffer
@ unittest/gunit/field-t.cc
Added Mock_table to be able to test get_timestamp() after
Field::is_null() was changed to test table->null_row before the
NULL bits
modified:
mysql-test/r/group_by.result
mysql-test/r/having.result
mysql-test/t/group_by.test
mysql-test/t/having.test
sql/field.h
sql/item_sum.cc
sql/sql_executor.cc
sql/sql_select.cc
sql/table.h
unittest/gunit/field-t.cc
3503 Tor Didriksen 2011-12-20
WL#5825 Using C++ Standard Library with MySQL code
Post-push cleanup: the min/max macros are gone, so don't comment about them.
It is still a good idea to include "my_config.h" before anything else.
@ unittest/gunit/opt_trace-t.cc
my_error_handler() should have C linkage.
modified:
unittest/gunit/bounded_queue-t.cc
unittest/gunit/bounds_checked_array-t.cc
unittest/gunit/cost_estimate-t.cc
unittest/gunit/dbug-t.cc
unittest/gunit/dynarray-t.cc
unittest/gunit/field-t.cc
unittest/gunit/filesort_buffer-t.cc
unittest/gunit/get_diagnostics-t.cc
unittest/gunit/gunit_test_main.cc
unittest/gunit/item-t.cc
unittest/gunit/mdl-t.cc
unittest/gunit/mdl_mytap-t.cc
unittest/gunit/my_regex-t.cc
unittest/gunit/opt_range-t.cc
unittest/gunit/opt_trace-t.cc
unittest/gunit/sql_list-t.cc
unittest/gunit/sql_plist-t.cc
unittest/gunit/stdcxx-t.cc
unittest/gunit/tap_event_listener.cc
unittest/gunit/test_utils.cc
unittest/gunit/thread_utils-t.cc
unittest/gunit/thread_utils.cc
=== modified file 'mysql-test/r/group_by.result'
--- a/mysql-test/r/group_by.result 2011-12-16 11:18:10 +0000
+++ b/mysql-test/r/group_by.result 2011-12-20 12:04:31 +0000
@@ -2204,3 +2204,40 @@ field1 field2
1 3
DROP VIEW v1;
DROP TABLE t1;
+#
+# Bug#13422961: WRONG RESULTS FROM SELECT WITH AGGREGATES AND
+# IMPLICIT GROUPING + MYISAM OR MEM
+#
+CREATE TABLE it (
+pk INT NOT NULL,
+col_int_nokey INT NOT NULL,
+PRIMARY KEY (pk)
+) ENGINE=MyISAM;
+CREATE TABLE ot (
+pk int(11) NOT NULL,
+col_int_nokey int(11) NOT NULL,
+PRIMARY KEY (pk)
+) ENGINE=MyISAM;
+INSERT INTO ot VALUES (10,8);
+
+SELECT col_int_nokey, MAX( pk )
+FROM ot
+WHERE (8, 1) IN ( SELECT pk, COUNT( col_int_nokey ) FROM it );
+col_int_nokey MAX( pk )
+NULL NULL
+
+DROP TABLE it,ot;
+#
+# Bug#13430588: WRONG RESULT FROM IMPLICITLY GROUPED QUERY WITH
+# CONST TABLE AND NO MATCHING ROWS
+#
+CREATE TABLE t1 (i INT) ENGINE=MyISAM;
+INSERT INTO t1 VALUES (1);
+CREATE TABLE t2 (j INT) ENGINE=MyISAM;
+INSERT INTO t2 VALUES (1),(2);
+
+SELECT i, j, COUNT(i) FROM t1 JOIN t2 WHERE j=3;
+i j COUNT(i)
+NULL NULL 0
+
+DROP TABLE t1,t2;
=== modified file 'mysql-test/r/having.result'
--- a/mysql-test/r/having.result 2011-11-25 14:07:13 +0000
+++ b/mysql-test/r/having.result 2011-12-20 12:04:31 +0000
@@ -633,4 +633,24 @@ HAVING x < 2;
x
DROP TABLE it,ot;
DROP FUNCTION f;
+#
+# Bug#11760517: MIN/MAX FAILS TO EVALUATE HAVING CONDITION,
+# RETURNS INCORRECT NULL RESULT
+#
+CREATE TABLE t1 (pk INT PRIMARY KEY, i4 INT);
+INSERT INTO t1 VALUES (2,7), (4,7), (6,2), (17,0);
+
+SELECT MIN(table1.i4), MIN(table2.pk) as min_pk
+FROM t1 as table1, t1 as table2
+WHERE table1.pk = 1;
+MIN(table1.i4) min_pk
+NULL NULL
+
+SELECT MIN(table1.i4), MIN(table2.pk) as min_pk
+FROM t1 as table1, t1 as table2
+WHERE table1.pk = 1
+HAVING min_pk <= 10;
+MIN(table1.i4) min_pk
+
+DROP TABLE t1;
End of 5.6 tests
=== modified file 'mysql-test/t/group_by.test'
--- a/mysql-test/t/group_by.test 2011-11-17 10:09:13 +0000
+++ b/mysql-test/t/group_by.test 2011-12-20 12:04:31 +0000
@@ -1538,3 +1538,45 @@ LIMIT 3;
DROP VIEW v1;
DROP TABLE t1;
+
+--echo #
+--echo # Bug#13422961: WRONG RESULTS FROM SELECT WITH AGGREGATES AND
+--echo # IMPLICIT GROUPING + MYISAM OR MEM
+--echo #
+
+CREATE TABLE it (
+ pk INT NOT NULL,
+ col_int_nokey INT NOT NULL,
+ PRIMARY KEY (pk)
+) ENGINE=MyISAM;
+
+CREATE TABLE ot (
+ pk int(11) NOT NULL,
+ col_int_nokey int(11) NOT NULL,
+ PRIMARY KEY (pk)
+) ENGINE=MyISAM;
+INSERT INTO ot VALUES (10,8);
+
+--echo
+SELECT col_int_nokey, MAX( pk )
+FROM ot
+WHERE (8, 1) IN ( SELECT pk, COUNT( col_int_nokey ) FROM it );
+
+--echo
+DROP TABLE it,ot;
+
+--echo #
+--echo # Bug#13430588: WRONG RESULT FROM IMPLICITLY GROUPED QUERY WITH
+--echo # CONST TABLE AND NO MATCHING ROWS
+--echo #
+CREATE TABLE t1 (i INT) ENGINE=MyISAM;
+INSERT INTO t1 VALUES (1);
+
+CREATE TABLE t2 (j INT) ENGINE=MyISAM;
+INSERT INTO t2 VALUES (1),(2);
+
+--echo
+SELECT i, j, COUNT(i) FROM t1 JOIN t2 WHERE j=3;
+
+--echo
+DROP TABLE t1,t2;
=== modified file 'mysql-test/t/having.test'
--- a/mysql-test/t/having.test 2011-11-25 14:07:13 +0000
+++ b/mysql-test/t/having.test 2011-12-20 12:04:31 +0000
@@ -667,4 +667,25 @@ HAVING x < 2;
DROP TABLE it,ot;
DROP FUNCTION f;
+--echo #
+--echo # Bug#11760517: MIN/MAX FAILS TO EVALUATE HAVING CONDITION,
+--echo # RETURNS INCORRECT NULL RESULT
+--echo #
+CREATE TABLE t1 (pk INT PRIMARY KEY, i4 INT);
+INSERT INTO t1 VALUES (2,7), (4,7), (6,2), (17,0);
+
+--echo
+SELECT MIN(table1.i4), MIN(table2.pk) as min_pk
+FROM t1 as table1, t1 as table2
+WHERE table1.pk = 1;
+
+--echo
+SELECT MIN(table1.i4), MIN(table2.pk) as min_pk
+FROM t1 as table1, t1 as table2
+WHERE table1.pk = 1
+HAVING min_pk <= 10;
+
+--echo
+DROP TABLE t1;
+
--echo End of 5.6 tests
=== modified file 'sql/field.h'
--- a/sql/field.h 2011-12-15 12:12:14 +0000
+++ b/sql/field.h 2011-12-20 12:04:31 +0000
@@ -642,14 +642,24 @@ public:
inline bool is_null(my_ptrdiff_t row_offset= 0)
{
/*
- If the field is NULLable, it has a valid null_ptr pointer, and its
- NULLity is recorded in the "null_bit" bit of null_ptr[row_offset].
- Otherwise, it can still be NULL, if it belongs to the inner table of an
- outer join and the row is NULL-complemented: that case is recorded in
- TABLE::null_row.
+ The table may have been marked as containing only NULL values
+ for all fields if it is a NULL-complemented row of an OUTER JOIN
+ or if the query is an implicitly grouped query (has aggregate
+ functions but no GROUP BY clause) with no qualifying rows. If
+ this is the case (in which TABLE::null_row is true), the field
+ is considered to be NULL.
+
+ Otherwise, if the field is NULLable, it has a valid null_ptr
+ pointer, and its NULLity is recorded in the "null_bit" bit of
+ null_ptr[row_offset].
*/
- return null_ptr ? (null_ptr[row_offset] & null_bit ? 1 : 0) :
- table->null_row;
+ if (table->null_row)
+ return true;
+
+ if (null_ptr)
+ return (null_ptr[row_offset] & null_bit);
+
+ return false;
}
inline bool is_real_null(my_ptrdiff_t row_offset= 0)
{ return null_ptr ? (null_ptr[row_offset] & null_bit ? 1 : 0) : 0; }
=== modified file 'sql/item_sum.cc'
--- a/sql/item_sum.cc 2011-12-15 15:15:37 +0000
+++ b/sql/item_sum.cc 2011-12-20 12:04:31 +0000
@@ -1057,7 +1057,7 @@ void Aggregator_distinct::endup()
{
/*
We don't have a tree only if 'setup()' hasn't been called;
- this is the case of sql_select.cc:return_zero_rows.
+ this is the case of sql_executor.cc:return_zero_rows.
*/
if (tree)
table->field[0]->set_notnull();
=== modified file 'sql/sql_executor.cc'
--- a/sql/sql_executor.cc 2011-12-16 14:39:02 +0000
+++ b/sql/sql_executor.cc 2011-12-20 12:04:31 +0000
@@ -43,10 +43,9 @@ using std::max;
using std::min;
static void disable_sorted_access(JOIN_TAB* join_tab);
-static int return_zero_rows(JOIN *join, select_result *res,TABLE_LIST *tables,
- List<Item> &fields, bool send_row,
- ulonglong select_options, const char *info,
- Item *having);
+static void return_zero_rows(JOIN *join, List<Item> &fields);
+static void save_const_null_info(JOIN *join, table_map *save_nullinfo);
+static void restore_const_null_info(JOIN *join, table_map save_nullinfo);
static int do_select(JOIN *join,List<Item> *fields,TABLE *tmp_table,
Procedure *proc);
@@ -197,12 +196,7 @@ JOIN::exec()
if (zero_result_cause)
{
- (void) return_zero_rows(this, result, select_lex->leaf_tables,
- *columns_list,
- send_row_on_empty_set(),
- select_options,
- zero_result_cause,
- having);
+ return_zero_rows(this, *columns_list);
DBUG_VOID_RETURN;
}
@@ -1354,42 +1348,51 @@ static void update_const_equal_items(Ite
}
}
+/**
+ For some reason (impossible WHERE clause etc), the tables cannot
+ possibly contain any rows that will be in the result. This function
+ is used to return with a result based on no matching rows (i.e., an
+ empty result or one row with aggregates calculated without using
+ rows in the case of implicit grouping) before the execution of
+ nested loop join.
-static int
-return_zero_rows(JOIN *join, select_result *result,TABLE_LIST *tables,
- List<Item> &fields, bool send_row, ulonglong select_options,
- const char *info, Item *having)
+ @param join The join that does not produce a row
+ @param fields Fields in result
+*/
+static void
+return_zero_rows(JOIN *join, List<Item> &fields)
{
DBUG_ENTER("return_zero_rows");
join->join_free();
- if (send_row)
- {
- for (TABLE_LIST *table= tables; table; table= table->next_leaf)
- mark_as_null_row(table->table); // All fields are NULL
- if (having && having->val_int() == 0)
- send_row=0;
- }
- if (!(result->send_result_set_metadata(fields,
- Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF)))
+ if (!(join->result->send_result_set_metadata(fields,
+ Protocol::SEND_NUM_ROWS |
+ Protocol::SEND_EOF)))
{
bool send_error= FALSE;
- if (send_row)
+ if (join->send_row_on_empty_set())
{
+ // Mark tables as containing only NULL values
+ for (uint tableno= 0; tableno < join->tables; tableno++)
+ mark_as_null_row((join->join_tab+tableno)->table);
+
+ // Calculate aggregate functions for no rows
List_iterator_fast<Item> it(fields);
Item *item;
while ((item= it++))
- item->no_rows_in_result();
- send_error= result->send_data(fields);
+ item->no_rows_in_result();
+
+ if (!join->having || join->having->val_int())
+ send_error= join->result->send_data(fields);
}
if (!send_error)
- result->send_eof(); // Should be safe
+ join->result->send_eof(); // Should be safe
}
/* Update results for FOUND_ROWS */
join->thd->set_examined_row_count(0);
join->thd->limit_found_rows= 0;
- DBUG_RETURN(0);
+ DBUG_VOID_RETURN;
}
/**
@@ -1544,8 +1547,17 @@ do_select(JOIN *join,List<Item> *fields,
{
if (!join->having || join->having->val_int())
{
+ // Mark tables as containing only NULL values
+ join->clear();
+
+ // Calculate aggregate functions for no rows
List<Item> *columns_list= (procedure ? &join->procedure_fields_list :
fields);
+ List_iterator_fast<Item> it(*columns_list);
+ Item *item;
+ while ((item= it++))
+ item->no_rows_in_result();
+
rc= join->result->send_data(*columns_list);
}
}
@@ -3388,12 +3400,25 @@ end_send_group(JOIN *join, JOIN_TAB *joi
}
else
{
- if (!join->first_record)
- {
+ table_map save_nullinfo= 0;
+ if (!join->first_record)
+ {
+ /*
+ If this is a subquery, we need to save and later restore
+ the const table NULL info before clearing the tables
+ because the following executions of the subquery do not
+ reevaluate constant fields. @see save_const_null_info
+ and restore_const_null_info
+ */
+ if (join->select_lex->master_unit()->item && join->const_tables)
+ save_const_null_info(join, &save_nullinfo);
+
+ // Mark tables as containing only NULL values
+ join->clear();
+
+ // Calculate aggregate functions for no rows
List_iterator_fast<Item> it(*join->fields);
Item *item;
- /* No matching rows for group function */
- join->clear();
while ((item= it++))
item->no_rows_in_result();
@@ -3411,6 +3436,9 @@ end_send_group(JOIN *join, JOIN_TAB *joi
if (join->rollup_send_data((uint) (idx+1)))
error= 1;
}
+ if (save_nullinfo)
+ restore_const_null_info(join, save_nullinfo);
+
}
if (error > 0)
DBUG_RETURN(NESTED_LOOP_ERROR); /* purecov: inspected */
@@ -3669,11 +3697,34 @@ end_write_group(JOIN *join, JOIN_TAB *jo
int send_group_parts= join->send_group_parts;
if (idx < send_group_parts)
{
- if (!join->first_record)
- {
- /* No matching rows for group function */
- join->clear();
- }
+ table_map save_nullinfo= 0;
+ if (!join->first_record)
+ {
+ // Dead code or we need a test case for this branch
+ DBUG_ASSERT(false);
+ /*
+ If this is a subquery, we need to save and later restore
+ the const table NULL info before clearing the tables
+ because the following executions of the subquery do not
+ reevaluate constant fields. @see save_const_null_info
+ and restore_const_null_info
+ */
+ if (join->select_lex->master_unit()->item && join->const_tables)
+ save_const_null_info(join, &save_nullinfo);
+
+ // Mark tables as containing only NULL values
+ join->clear();
+
+ // Calculate aggregate functions for no rows
+ List<Item> *columns_list= (join->procedure ?
+ &join->procedure_fields_list :
+ join->fields);
+ List_iterator_fast<Item> it(*columns_list);
+ Item *item;
+ while ((item= it++))
+ item->no_rows_in_result();
+
+ }
copy_sum_funcs(join->sum_funcs,
join->sum_funcs_end[send_group_parts]);
if (!join->having || join->having->val_int())
@@ -3691,6 +3742,9 @@ end_write_group(JOIN *join, JOIN_TAB *jo
if (join->rollup_write_data((uint) (idx+1), table))
DBUG_RETURN(NESTED_LOOP_ERROR);
}
+ if (save_nullinfo)
+ restore_const_null_info(join, save_nullinfo);
+
if (end_of_records)
DBUG_RETURN(NESTED_LOOP_OK);
}
@@ -4705,6 +4759,79 @@ change_refs_to_tmp_fields(THD *thd, Ref_
/**
+ Save NULL-row info for constant tables. Used in conjunction with
+ restore_const_null_info() to restore constant table null_row and
+ status values after temporarily marking rows as NULL. This is only
+ done for const tables in subqueries because these values are not
+ recalculated on next execution of the subquery.
+
+ @param join The join for which const tables are about to be
+ marked as containing only NULL values
+ @param[out] save_nullinfo Const tables that have null_row=false and
+ STATUS_NULL_ROW set are tagged in this
+ table_map so that the value can be
+ restored by restore_const_null_info()
+
+ @see mark_as_null_row
+ @see restore_const_null_info
+*/
+static void save_const_null_info(JOIN *join, table_map *save_nullinfo)
+{
+ DBUG_ASSERT(join->const_tables);
+
+ for (uint tableno= 0; tableno < join->const_tables; tableno++)
+ {
+ TABLE *tbl= (join->join_tab+tableno)->table;
+ /*
+ tbl->status and tbl->null_row must be in sync: either both set
+ or none set. Otherwise, an additional table_map parameter is
+ needed to save/restore_const_null_info() these separately
+ */
+ DBUG_ASSERT(tbl->null_row ? (tbl->status & STATUS_NULL_ROW) :
+ !(tbl->status & STATUS_NULL_ROW));
+
+ if (!tbl->null_row)
+ *save_nullinfo|= tbl->map;
+ }
+}
+
+/**
+ Restore NULL-row info for constant tables. Used in conjunction with
+ save_const_null_info() to restore constant table null_row and status
+ values after temporarily marking rows as NULL. This is only done for
+ const tables in subqueries because these values are not recalculated
+ on next execution of the subquery.
+
+ @param join The join for which const tables have been
+ marked as containing only NULL values
+ @param save_nullinfo Const tables that had null_row=false and
+ STATUS_NULL_ROW set when
+ save_const_null_info() was called
+
+ @see mark_as_null_row
+ @see save_const_null_info
+*/
+static void restore_const_null_info(JOIN *join, table_map save_nullinfo)
+{
+ DBUG_ASSERT(join->const_tables && save_nullinfo);
+
+ for (uint tableno= 0; tableno < join->const_tables; tableno++)
+ {
+ TABLE *tbl= (join->join_tab+tableno)->table;
+ if ((save_nullinfo & tbl->map))
+ {
+ /*
+ The table had null_row=false and STATUS_NULL_ROW set when
+ save_const_null_info was called
+ */
+ tbl->null_row= false;
+ tbl->status&= ~STATUS_NULL_ROW;
+ }
+ }
+}
+
+
+/**
@} (end of group Query_Executor)
*/
=== modified file 'sql/sql_select.cc'
--- a/sql/sql_select.cc 2011-12-16 11:20:30 +0000
+++ b/sql/sql_select.cc 2011-12-20 12:04:31 +0000
@@ -3209,18 +3209,6 @@ ORDER *simple_remove_const(ORDER *order,
}
-/*
- used only in JOIN::clear
-*/
-static void clear_tables(JOIN *join)
-{
- /*
- must clear only the non-const tables, as const tables
- are not re-calculated.
- */
- for (uint i=join->const_tables ; i < join->tables ; i++)
- mark_as_null_row(join->all_tables[i]); // All fields are NULL
-}
/*
@@ -4593,7 +4581,9 @@ bool JOIN::rollup_make_fields(List<Item>
void JOIN::clear()
{
- clear_tables(this);
+ for (uint tableno= 0; tableno < this->tables; tableno++)
+ mark_as_null_row((join_tab+tableno)->table);
+
copy_fields(&tmp_table_param);
if (sum_funcs)
=== modified file 'sql/table.h'
--- a/sql/table.h 2011-12-16 14:39:02 +0000
+++ b/sql/table.h 2011-12-20 12:04:31 +0000
@@ -2335,7 +2335,6 @@ inline void mark_as_null_row(TABLE *tabl
{
table->null_row=1;
table->status|=STATUS_NULL_ROW;
- memset(table->null_flags, 255, table->s->null_bytes);
}
bool is_simple_order(ORDER *order);
=== modified file 'unittest/gunit/field-t.cc'
--- a/unittest/gunit/field-t.cc 2011-12-20 09:51:05 +0000
+++ b/unittest/gunit/field-t.cc 2011-12-20 12:04:31 +0000
@@ -69,6 +69,17 @@ static void compareMysqlTime(const MYSQL
EXPECT_EQ(first.time_type, second.time_type);
}
+class Mock_table : public TABLE
+{
+public:
+ Mock_table(THD *thd)
+ {
+ null_row= false;
+ read_set= 0;
+ in_use= thd;
+ }
+};
+
// A mock Protocol class to be able to test Field::send_binary
// It just verifies that store_time has been passed what is expected
class Mock_protocol : public Protocol
@@ -256,6 +267,8 @@ TEST_F(FieldTest, FieldTimef)
// Not testing store(const char, uint, const CHARSET_INFO *, enum_check_fields)
// it requires a mock table
+ Mock_table m_table(thd());
+ f->table= &m_table;
struct timeval tv;
int warnings= 0;
EXPECT_EQ(0, f->get_timestamp(&tv, &warnings));
No bundle (reason: useless for push emails).
| Thread |
|---|
| • bzr push into mysql-trunk branch (jorgen.loland:3503 to 3504) Bug#11760517Bug#13422961 Bug#13430588 | Jorgen Loland | 21 Dec |
