Fix sorting of NULL values (Should always be first)

Fix problem with HAVING and MAX() IS NOT NULL


Docs/manual.texi:
  Changelog & NULL usage with ORDER BY
client/mysqldump.c:
  Cleanup disable keys
mysql-test/r/distinct.result:
  Fix results after ORDER BY with NULL fix
mysql-test/r/group_by.result:
  Fix results after ORDER BY with NULL fix
mysql-test/r/having.result:
  Testcase for bug with HAVING
mysql-test/t/distinct.test:
  Test for DISTINCT + ORDER BY DESC bug
mysql-test/t/having.test:
  Test of HAVING and MAX IS NOT NULL
sql/filesort.cc:
  Fix sorting of NULL values (Should always be first)
sql/item.h:
  Fix problem with HAVING and MAX() IS NOT NULL
sql/item_sum.h:
  Fix problem with HAVING and MAX() IS NOT NULL
sql/opt_range.cc:
  Fix problem with HAVING and MAX() IS NOT NULL
sql/opt_range.h:
  Fix sorting of NULL values
sql/sql_select.cc:
  Fix sorting of ORDER BY ... DESC on NULL values.

Docs/manual.texi

@@ -8146,6 +8146,9 @@ version 4.0;
 
 @itemize @bullet
 @item
+Use @code{ORDER BY column DESC} now always sorts @code{NULL} values
+first; In 3.23 this was not always consistent.
+@item
 @code{SHOW INDEX} has 2 columns more (@code{Null} and @code{Index_type})
 than it had in 3.23.
 @item
@@ -12661,9 +12664,15 @@ mysql> SELECT 1 IS NULL, 1 IS NOT NULL;
 +-----------+---------------+
 @end example
 
+Note that two @code{NULL} are compared as equal is when you do an
+@code{GROUP BY}.
+
 In MySQL, 0 or @code{NULL} means false and anything else means true.
 The default truth value from a boolean operation is 1.
 
+When doing an @code{ORDER BY}, @code{NULL} values are always sorted first,
+even if you are using @code{DESC}.
+
 This special treatment of @code{NULL} is why, in the previous section, it
 was necessary to determine which animals are no longer alive using
 @code{death IS NOT NULL} instead of @code{death <> NULL}.
@@ -13191,7 +13200,7 @@ mysql> DESCRIBE pet;
 @end example
 
 @code{Field} indicates the column name, @code{Type} is the data type for
-the column, @code{Null} indicates whether or not the column can contain
+the column, @code{NULL} indicates whether or not the column can contain
 @code{NULL} values, @code{Key} indicates whether or not the column is
 indexed, and @code{Default} specifies the column's default value.
 
@@ -16481,8 +16490,10 @@ password will be set to the password specified by the @code{IDENTIFIED BY}
 clause, if one is given.  If the user already had a password, it is replaced
 by the new one.
 
-Optional @code{PASSWORD} changes behaviour of @code{IDENTIFIED BY} from
-accepting plain password to accept encrypted password as argument.
+If you don't want to send the password in clear text you can use the
+@code{PASSWORD} option followed by a scrambled password from SQL
+function @code{PASSWORD()} or the C API function
+@code{make_scrambled_password(char *to, const char *password)}.
 
 @strong{Warning:} If you create a new user but do not specify an
 @code{IDENTIFIED BY} clause, the user has no password.  This is insecure.
@@ -25531,7 +25542,13 @@ You have different @code{ORDER BY} and @code{GROUP BY} expressions.
 
 @item
 The used table index is an index type that doesn't store rows in order.
-(Like index in @code{HEAP} tables).
+(Like the @code{HASH} index in @code{HEAP} tables).
+
+@item
+The index colum may contain @code{NULL} values and one is using
+@code{ORDER BY ... DESC}.  This is because in SQL @code{NULL} values is
+always sorted before normal values, independent of you are using
+@code{DESC} or not.
 @end itemize
 
 
@@ -26466,6 +26483,9 @@ probably much faster, as this will require us to do much fewer seeks.)
 Note that if such a query uses @code{LIMIT} to only retrieve
 part of the rows, MySQL will use an index anyway, as it can
 much more quickly find the few rows to return in the result.
+@item
+If the index range may contain @code{NULL} values and you are using
+@code{ORDER BY ... DESC}
 @end itemize
 
 @node Indexes, Multiple-column indexes, MySQL indexes, Optimising Database Structure
@@ -29975,7 +29995,7 @@ mysql> select 2 > 2;
 @cindex @code{NULL}, testing for null
 @findex <=> (Equal to)
 @item <=>
-Null safe equal:
+NULL safe equal:
 @example
 mysql> select 1 <=> 1, NULL <=> NULL, 1 <=> NULL;
         -> 1 1 0
@@ -48618,6 +48638,12 @@ Our TODO section contains what we plan to have in 4.0. @xref{TODO MySQL 4.0}.
 
 @itemize @bullet
 @item
+Use @code{ORDER BY column DESC} now sorts @code{NULL} values first.
+@item
+Fixed bug in @code{SELECT DISTINCT ... ORDER BY DESC} optimization.
+@item
+Fixed bug in @code{... HAVING 'GROUP_FUNCTION'(xxx) IS [NOT] NULL}.
+@item
 Allow numeric user id to @code{mysqld --user=#}.
 @item
 Fixed a bug where @code{SQL_CALC_ROWS} returned a wrong value when used

client/mysqldump.c

@@ -35,7 +35,7 @@
 ** and adapted to mysqldump 05/11/01 by Jani Tolonen
 */
 
-#define DUMP_VERSION "8.22"
+#define DUMP_VERSION "8.23"
 
 #include <my_global.h>
 #include <my_sys.h>
@@ -897,8 +897,6 @@ static uint getTableStructure(char *table, char* db)
       fputs(";\n", sql_file);
     }
   }
-  if (opt_disable_keys)
-    fprintf(sql_file,"\n/*!40000 ALTER TABLE %s DISABLE KEYS */;\n",table_name);
   if (cFlag)
   {
     strpos=strmov(strpos,") VALUES ");
@@ -1023,7 +1021,7 @@ static void dumpTable(uint numFields, char *table)
       strxmov(strend(query), " WHERE ",where,NullS);
     }
     if (!opt_xml)
-      fputs("\n\n", md_result_file);
+      fputs("\n", md_result_file);
     if (mysql_query(sock, query))
     {
       DBerror(sock, "when retrieving data from server");
@@ -1048,6 +1046,9 @@ static void dumpTable(uint numFields, char *table)
       return;
     }
 
+    if (opt_disable_keys)
+      fprintf(md_result_file,"/*!40000 ALTER TABLE %s DISABLE KEYS */;\n",
+	      quote_name(table, table_buff));
     if (opt_lock)
       fprintf(md_result_file,"LOCK TABLES %s WRITE;\n",
 	      quote_name(table,table_buff));
@@ -1207,11 +1208,11 @@ static void dumpTable(uint numFields, char *table)
       safe_exit(EX_CONSCHECK);
       return;
     }
-    if (opt_disable_keys)
-      fprintf(md_result_file,"\n/*!40000 ALTER TABLE %s ENABLE KEYS */;\n",
-                                            quote_name(table,table_buff));
     if (opt_lock)
       fputs("UNLOCK TABLES;\n", md_result_file);
+    if (opt_disable_keys)
+      fprintf(md_result_file,"/*!40000 ALTER TABLE %s ENABLE KEYS */;\n",
+	      quote_name(table,table_buff));
     if (opt_autocommit)
       fprintf(md_result_file, "commit;\n");
     mysql_free_result(res);

mysql-test/r/distinct.result

@@ -77,6 +77,7 @@ NULL	NULL
 10	VMT
 select id+0 as a,max(id),concat(facility) as b from t1 group by a order by b desc,a;
 a	max(id)	b
+NULL	NULL	NULL
 10	10	VMT
 9	9	SRV
 8	8	RV
@@ -89,7 +90,6 @@ a	max(id)	b
 1	1	/L
 -1	-1	
 0	0	
-NULL	NULL	NULL
 select id >= 0 and id <= 5 as grp,count(*) from t1 group by grp;
 grp	count(*)
 0	7
@@ -336,3 +336,16 @@ a	c
 4	NULL
 3	NULL
 drop table t1;
+create table t1 (a char(1), key(a)) type=myisam;
+insert into t1 values('1'),('1');
+select * from t1 where a >= '1';
+a
+1
+1
+select distinct a from t1 order by a desc;
+a
+1
+select distinct a from t1 where a >= '1' order by a desc;
+a
+1
+drop table t1;

mysql-test/r/group_by.result

@@ -226,7 +226,7 @@ key (score)
 INSERT INTO t1 VALUES (1,1,1),(2,2,2),(2,1,1),(3,3,3),(4,3,3),(5,3,3);
 explain select userid,count(*) from t1 group by userid desc;
 table	type	possible_keys	key	key_len	ref	rows	Extra
-t1	ALL	NULL	NULL	NULL	NULL	6	Using temporary
+t1	ALL	NULL	NULL	NULL	NULL	6	Using temporary; Using filesort
 select userid,count(*) from t1 group by userid desc;
 userid	count(*)
 3	3
@@ -244,6 +244,8 @@ spid	count(*)
 2	2
 select spid,count(*) from t1 where spid between 1 and 2 group by spid desc;
 spid	count(*)
+2	2
+1	1
 explain select sql_big_result spid,sum(userid) from t1 group by spid desc;
 table	type	possible_keys	key	key_len	ref	rows	Extra
 t1	ALL	NULL	NULL	NULL	NULL	6	Using filesort

mysql-test/r/having.result

@@ -44,3 +44,22 @@ AND start <= 999660;
 id	start	end	chr_strand
 133197	813898	813898	-1.0000
 drop table t1,t2;
+CREATE TABLE t1 (Fld1 int(11) default NULL,Fld2 int(11) default NULL);
+INSERT INTO t1 VALUES (1,10),(1,20),(2,NULL),(2,NULL),(3,50);
+select Fld1, max(Fld2) as q from t1 group by Fld1 having q is not null;
+Fld1	q
+1	20
+3	50
+select Fld1, max(Fld2) from t1 group by Fld1 having max(Fld2) is not null;
+Fld1	max(Fld2)
+1	20
+3	50
+select Fld1, max(Fld2) from t1 group by Fld1 having avg(Fld2) is not null;
+Fld1	max(Fld2)
+1	20
+3	50
+select Fld1, max(Fld2) from t1 group by Fld1 having std(Fld2) is not null;
+Fld1	max(Fld2)
+1	20
+3	50
+drop table t1;

mysql-test/t/distinct.test

@@ -207,3 +207,14 @@ insert into t1 (a) values (1),(2),(3),(4),(1),(2),(3),(4);
 select distinct a from t1 group by b,a having a > 2 order by a desc;
 select distinct a,c from t1 group by b,c,a having a > 2 order by a desc;
 drop table t1;
+
+#
+# Test problem with DISTINCT and ORDER BY DESC
+#
+
+create table t1 (a char(1), key(a)) type=myisam;
+insert into t1 values('1'),('1');
+select * from t1 where a >= '1'; 
+select distinct a from t1 order by a desc;
+select distinct a from t1 where a >= '1' order by a desc;
+drop table t1;

mysql-test/t/having.test

@@ -48,3 +48,15 @@ GROUP BY e.id
 HAVING chr_strand= -1 and end >= 0 
   AND start <= 999660;
 drop table t1,t2;
+
+#
+# Test problem with having and MAX() IS NOT NULL
+#
+
+CREATE TABLE t1 (Fld1 int(11) default NULL,Fld2 int(11) default NULL);
+INSERT INTO t1 VALUES (1,10),(1,20),(2,NULL),(2,NULL),(3,50);
+select Fld1, max(Fld2) as q from t1 group by Fld1 having q is not null;
+select Fld1, max(Fld2) from t1 group by Fld1 having max(Fld2) is not null;
+select Fld1, max(Fld2) from t1 group by Fld1 having avg(Fld2) is not null;
+select Fld1, max(Fld2) from t1 group by Fld1 having std(Fld2) is not null;
+drop table t1;

sql/filesort.cc

@@ -452,10 +452,7 @@ static void make_sortkey(register SORTPARAM *param,
       {
 	if (field->is_null())
 	{
-	  if (sort_field->reverse)
-	    bfill(to,sort_field->length+1,(char) 255);
-	  else
-	    bzero((char*) to,sort_field->length+1);
+	  bzero((char*) to,sort_field->length+1);
 	  to+= sort_field->length+1;
 	  continue;
 	}

sql/item.h

@@ -343,6 +343,11 @@ class Item_ref :public Item_ident
     null_value=(*ref)->null_value;
     return tmp;
   }
+  bool is_null()
+  {
+    (void) (*ref)->val_int_result();
+    return (*ref)->null_value;
+  }
   bool get_date(TIME *ltime,bool fuzzydate)
   {  
     return (null_value=(*ref)->get_date(ltime,fuzzydate));

sql/item_sum.h

@@ -64,6 +64,7 @@ class Item_sum :public Item_result_field
   { return new Item_field(field);}
   table_map used_tables() const { return ~(table_map) 0; } /* Not used */
   bool const_item() const { return 0; }
+  bool is_null() { return null_value; }
   void update_used_tables() { }
   void make_field(Send_field *field);
   void print(String *str);
@@ -202,6 +203,7 @@ class Item_avg_field :public Item_result_field
   enum Type type() const { return FIELD_AVG_ITEM; }
   double val();
   longlong val_int() { return (longlong) val(); }
+  bool is_null() { (void) val_int(); return null_value; }
   String *val_str(String*);
   void make_field(Send_field *field);
   void fix_length_and_dec() {}
@@ -239,6 +241,7 @@ class Item_std_field :public Item_result_field
   double val();
   longlong val_int() { return (longlong) val(); }
   String *val_str(String*);
+  bool is_null() { (void) val_int(); return null_value; }
   void make_field(Send_field *field);
   void fix_length_and_dec() {}
 };

sql/opt_range.cc

@@ -2523,13 +2523,13 @@ int QUICK_SELECT::cmp_next(QUICK_RANGE *range)
 
 
 /*
- * This is a hack: we inherit from QUICK_SELECT so that we can use the
- * get_next() interface, but we have to hold a pointer to the original
- * QUICK_SELECT because its data are used all over the place.  What
- * should be done is to factor out the data that is needed into a base
- * class (QUICK_SELECT), and then have two subclasses (_ASC and _DESC)
- * which handle the ranges and implement the get_next() function.  But
- * for now, this seems to work right at least.
+  This is a hack: we inherit from QUICK_SELECT so that we can use the
+  get_next() interface, but we have to hold a pointer to the original
+  QUICK_SELECT because its data are used all over the place.  What
+  should be done is to factor out the data that is needed into a base
+  class (QUICK_SELECT), and then have two subclasses (_ASC and _DESC)
+  which handle the ranges and implement the get_next() function.  But
+  for now, this seems to work right at least.
  */
 
 QUICK_SELECT_DESC::QUICK_SELECT_DESC(QUICK_SELECT *q, uint used_key_parts)
@@ -2538,6 +2538,7 @@ QUICK_SELECT_DESC::QUICK_SELECT_DESC(QUICK_SELECT *q, uint used_key_parts)
   bool not_read_after_key = file->option_flag() & HA_NOT_READ_AFTER_KEY;
   QUICK_RANGE *r;
 
+  it.rewind();
   for (r = it++; r; r = it++)
   {
     rev_ranges.push_front(r);

sql/opt_range.h

@@ -77,6 +77,7 @@ class QUICK_SELECT {
   void reset(void) { next=0; it.rewind(); }
   int init() { return error=file->index_init(index); }
   virtual int get_next();
+  virtual bool reverse_sorted() { return 0; }
   int cmp_next(QUICK_RANGE *range);
   bool unique_key_range();
 };
@@ -87,6 +88,7 @@ class QUICK_SELECT_DESC: public QUICK_SELECT
 public:
   QUICK_SELECT_DESC(QUICK_SELECT *q, uint used_key_parts);
   int get_next();
+  bool reverse_sorted() { return 1; }
 private:
   int cmp_prev(QUICK_RANGE *range);
   bool range_reads_after_key(QUICK_RANGE *range);
@@ -96,6 +98,7 @@ class QUICK_SELECT_DESC: public QUICK_SELECT
   List_iterator<QUICK_RANGE> rev_it;
 };
 
+
 class SQL_SELECT :public Sql_alloc {
  public:
   QUICK_SELECT *quick;		// If quick-select used

sql/sql_select.cc

@@ -594,8 +594,7 @@ mysql_select(THD *thd,TABLE_LIST *tables,List<Item> &fields,COND *conds,
 				  HA_POS_ERROR : thd->select_limit,0))))
       order=0;
     select_describe(&join,need_tmp,
-		    (order != 0 &&
-		     (!need_tmp || order != group || simple_group)),
+		    order != 0 && !skip_sort_order,
 		    select_distinct);
     error=0;
     goto err;
@@ -5431,7 +5430,16 @@ static uint find_shortest_key(TABLE *table, key_map usable_keys)
 }
 
 
-/* Return 1 if we don't have to do file sorting */
+/*
+  Test if we can skip the ORDER BY by using an index.
+
+  If we can use an index, the JOIN_TAB / tab->select struct
+  is changed to use the index.
+
+  Return:
+     0 We have to use filesort to do the sorting
+     1 We can use an index.
+*/
 
 static bool
 test_if_skip_sort_order(JOIN_TAB *tab,ORDER *order,ha_rows select_limit,
@@ -5477,15 +5485,22 @@ test_if_skip_sort_order(JOIN_TAB *tab,ORDER *order,ha_rows select_limit,
       {
 	if (select && select->quick)
 	{
-	  // ORDER BY range_key DESC
-	  QUICK_SELECT_DESC *tmp=new QUICK_SELECT_DESC(select->quick,
-						       used_key_parts);
-	  if (!tmp || tmp->error)
+	  /*
+	    Don't reverse the sort order, if it's already done.
+	    (In some cases test_if_order_by_key() can be called multiple times
+	  */
+	  if (!select->quick->reverse_sorted())
 	  {
-	    delete tmp;
-	    DBUG_RETURN(0);		// Reverse sort not supported
+	    // ORDER BY range_key DESC
+	    QUICK_SELECT_DESC *tmp=new QUICK_SELECT_DESC(select->quick,
+							 used_key_parts);
+	    if (!tmp || tmp->error)
+	    {
+	      delete tmp;
+	      DBUG_RETURN(0);		// Reverse sort not supported
+	    }
+	    select->quick=tmp;
 	  }
-	  select->quick=tmp;
 	  DBUG_RETURN(1);
 	}
 	if (tab->ref.key_parts < used_key_parts)
@@ -7028,7 +7043,7 @@ static void select_describe(JOIN *join, bool need_tmp_table, bool need_order,
       net_store_null(packet);
       net_store_null(packet);
     }
-    sprintf(buff,"%.0f",join->best_positions[i].records_read);
+    sprintf(buff,"%.0f",(double) join->best_positions[i].records_read);
     net_store_data(packet,buff);
     my_bool key_read=table->key_read;
     if (tab->type == JT_NEXT &&