maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Sergei,

== TL;DR ==
I have debugged that LEFT JOIN problem. The fix is not pretty, but
correct. I will now push it into the 10.0 tree.

== Details ==

So, the commit changes 

-    return (table->null_row ? TRUE :
-            null_ptr ? MY_TEST(null_ptr[row_offset] & null_bit) : 0);
+    return real_maybe_null() ?
+      MY_TEST(null_ptr[row_offset] & null_bit) : table->null_row;

The patch changes the behavior for NULLable fields.  
Suppose
* real_maybe_null()= TRUE
* table->null_row==TRUE  // the table has a NULL-complemented row
* (null_ptr[row_offset] & null_bit) == FALSE // however the value 
                                            // is not null.

the old code will return TRUE 
the new code will return FALSE

The new code may seem wrong at first glance (it's a NULL-complemented row, why 
we return FALSE from is_null()?) However, if we look where table->null_row is
set to true, we will find that it is done here:

inline void mark_as_null_row(TABLE *table)
{
  table->null_row=1;
  table->status|=STATUS_NULL_ROW;
  bfill(table->null_flags,table->s->null_bytes,255);
}

The function also sets all NULL-bits, so normally one can assume that 

  (table->null_row=TRUE) =>  ((null_ptr[..] & null_bit) == TRUE)

However, there is a case when the above does not hold. 
It happens when we run GROUP BY query, and GROUP BY is satisfied by 
scanning an index which produces rows in group order.

In that case, the following can occur:

consider the query from the testcase:
 
  SELECT ... 
  FROM table_one a LEFT JOIN table_two b ON a.oid=b.oid 
  GROUP BY a.oid


* we read from a row (1,3)
* we make index lookup in table b and find row (3). 
* row combination a(1,3)-b(3) passes the WHERE clause, 
  GROUP BY execution code records it as current row.

* there are no other matches in table b, so we get back
  to reading table a.
* we read row a(1,4)
* we look for matches in table b. there are no matches,
  so we produce a NULL-complemented row b(NULLs)

* row combination a(1,4)-b(NULLs) is passed to the GROUP BY 
  runtime. The runtime sees that we are now in a different
  group, and figures that it should pass the row combination
  representing the previous group into query output.
* It attempts to pass the row combination 

    a(1,3)-b(3)         (*)

  into query output.
  However, the current row for table b is already the NULL-complemented row.

the row combination (*) is stored in the "result_field" objects (and not 
in "regular field" objects which refer to table->record[0]).

It seems wrong that we look at result_field (which points to a row that's 
different from table->record[0]), and at the same time we are looking at
table->null_row, which refers to the record in table->record[0]. 

However, "result_field" storage has no place for table->null_row bit. 

The fix basically means:
"for NULL-able fields, field's NULL-bit should override table->null_row".

Considering the above, it is correct (albeit ugly).

Now, what about GROUP BY and NOT NULL fields? I discovered the following:
if a table has table->maybe_null, then Field objects that refer to such table
and are pointed by Item_field::result_field, are always NULLable.

(code-wise, check out setup_copy_fields(), these calls:

  item->result_field=field->new_field(thd->mem_root,field->table, 1);
  item->result_field->move_field(copy->to_ptr,copy->to_null_ptr,1);

result_field object of NOT NULL column is null-able.



On Fri, Jan 23, 2015 at 08:25:59AM -0800, Nirbhay Choubey wrote:
> At lp:~maria-captains/maria/10.0
> 
> ------------------------------------------------------------
> revno: 4577
> revision-id: nirbhay@xxxxxxxxxxx-20150123162551-h88uxn780ivj5k2n
> parent: sergii@xxxxxxxxx-20150119153032-fqrmxjr13z0p1ly0
> committer: Nirbhay Choubey <nirbhay@xxxxxxxxxxx>
> branch nick: b5719-10.0
> timestamp: Fri 2015-01-23 11:25:51 -0500
> message:
>   MDEV-5719: Wrong result with GROUP BY and LEFT OUTER JOIN
>   
>   Merged revision 5224 from mysql-5.6 and added a test case.
>   ..
>   revno: 5224
>   committer: Sergey Glukhov <sergey.glukhov@xxxxxxxxxx>
>   branch nick: mysql-5.6
>   timestamp: Wed 2013-06-19 14:24:08 +0400
>   message:
>     Bug#16620047 INCORRECT QUERY RESULT (AFTER SERVER UPGRADE)
>   ..

> === modified file 'mysql-test/r/group_by_innodb.result'
> --- a/mysql-test/r/group_by_innodb.result	2014-01-26 20:49:11 +0000
> +++ b/mysql-test/r/group_by_innodb.result	2015-01-23 16:25:51 +0000
> @@ -57,3 +57,26 @@
>  NULL	11.1,22.2
>  DROP TABLE t1;
>  End of 5.5 tests
> +#
> +# MDEV-5719: Wrong result with GROUP BY and LEFT OUTER JOIN
> +#
> +CREATE TABLE table_one (oidGroup INT, oid INT PRIMARY KEY)ENGINE=INNODB;
> +INSERT INTO table_one VALUES (1,1),(1,2),(1,3),(1,4);
> +CREATE TABLE table_two (oid INT PRIMARY KEY)ENGINE=INNODB;
> +INSERT INTO table_two VALUES (3);
> +SELECT a.oidGroup, a.oid, b.oid FROM table_one a LEFT JOIN table_two b ON
> +a.oid=b.oid WHERE a.oidGroup=1;
> +oidGroup	oid	oid
> +1	1	NULL
> +1	2	NULL
> +1	3	3
> +1	4	NULL
> +SELECT a.oidGroup, a.oid, b.oid FROM table_one a LEFT JOIN table_two b ON
> +a.oid=b.oid WHERE a.oidGroup=1 GROUP BY a.oid;
> +oidGroup	oid	oid
> +1	1	NULL
> +1	2	NULL
> +1	3	3
> +1	4	NULL
> +DROP TABLE table_one, table_two;
> +# End of tests
> 
> === modified file 'mysql-test/t/group_by_innodb.test'
> --- a/mysql-test/t/group_by_innodb.test	2014-01-26 20:49:11 +0000
> +++ b/mysql-test/t/group_by_innodb.test	2015-01-23 16:25:51 +0000
> @@ -67,3 +67,22 @@
>  
>  --echo End of 5.5 tests
>  
> +--echo #
> +--echo # MDEV-5719: Wrong result with GROUP BY and LEFT OUTER JOIN
> +--echo #
> +CREATE TABLE table_one (oidGroup INT, oid INT PRIMARY KEY)ENGINE=INNODB;
> +INSERT INTO table_one VALUES (1,1),(1,2),(1,3),(1,4);
> +
> +CREATE TABLE table_two (oid INT PRIMARY KEY)ENGINE=INNODB;
> +INSERT INTO table_two VALUES (3);
> +
> +# Returns a value
> +SELECT a.oidGroup, a.oid, b.oid FROM table_one a LEFT JOIN table_two b ON
> +a.oid=b.oid WHERE a.oidGroup=1;
> +
> +SELECT a.oidGroup, a.oid, b.oid FROM table_one a LEFT JOIN table_two b ON
> +a.oid=b.oid WHERE a.oidGroup=1 GROUP BY a.oid;
> +
> +DROP TABLE table_one, table_two;
> +
> +--echo # End of tests
> 
> === modified file 'sql/field.h'
> --- a/sql/field.h	2014-10-20 12:42:00 +0000
> +++ b/sql/field.h	2015-01-23 16:25:51 +0000
> @@ -656,21 +656,28 @@
>    inline bool is_null(my_ptrdiff_t row_offset= 0) const
>    {
>      /*
> +      If the field is NULLable, it returns NULLity based
> +      on null_ptr[row_offset] value. Otherwise it returns
> +      NULL flag depending on TABLE::null_row value.
> +
>        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.
> +      this is the case (in which TABLE::null_row is true) and the
> +      field is not nullable, the field is considered to be NULL.
> +
> +      Do not change the order of testing. Fields may be associated
> +      with a TABLE object without being part of the current row.
> +      For NULL value check to work for these fields, they must
> +      have a valid null_ptr, and this pointer must be checked before
> +      TABLE::null_row.
> +
>        Note that if a table->null_row is set then also all null_bits are
>        set for the row.
> -
> -      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 (table->null_row ? TRUE :
> -            null_ptr ? MY_TEST(null_ptr[row_offset] & null_bit) : 0);
> +    return real_maybe_null() ?
> +      MY_TEST(null_ptr[row_offset] & null_bit) : table->null_row;
>    }
>    inline bool is_real_null(my_ptrdiff_t row_offset= 0) const
>      { return null_ptr ? (null_ptr[row_offset] & null_bit ? 1 : 0) : 0; }
> 

> _______________________________________________
> commits mailing list
> commits@xxxxxxxxxxx
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits


-- 
BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog