MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:kgeorge Date:December 21 2007 4:24pm
Subject:bk commit into 5.0 tree (gkodinov:1.2594) BUG#16854
View as plain text  
Below is the list of changes that have just been committed into a local
5.0 repository of kgeorge. When kgeorge does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet@stripped, 2007-12-21 18:24:05+02:00, gkodinov@stripped +5 -0
  Bug #16854: IF( 1, A, B ) * expr != IF( <true>, A, B ) * expr. 
    --> Heavy rounding errors.
  When calculating aggregated functions the optimizer is storing
  intermediate function calculation results. It used a string 
  buffer to hold these results. This caused the problem, because
  when converting intermediate results to strings their designated
  precision was limited.
  Fixed by removing Item_copy_string and replacing it with 
  Item_cache_* hierarchy that has ints, floating point etc buffers
  so the precision of the intermediate results is not lost.

  mysql-test/r/func_group.result@stripped, 2007-12-21 18:24:00+02:00, gkodinov@stripped +9 -0
    Bug #16854: test case

  mysql-test/t/func_group.test@stripped, 2007-12-21 18:24:01+02:00, gkodinov@stripped +16 -0
    Bug #16854: test case

  sql/item.cc@stripped, 2007-12-21 18:24:01+02:00, gkodinov@stripped +0 -32
    Bug #16854: Remove Item_copy_string

  sql/item.h@stripped, 2007-12-21 18:24:01+02:00, gkodinov@stripped +18 -46
    Bug #16854: Remove Item_copy_string and replace with Item_cache_*

  sql/sql_select.cc@stripped, 2007-12-21 18:24:02+02:00, gkodinov@stripped +14 -16
    Bug #16854: Remove Item_copy_string and replace with Item_cache_*

diff -Nrup a/mysql-test/r/func_group.result b/mysql-test/r/func_group.result
--- a/mysql-test/r/func_group.result	2007-11-01 18:36:24 +02:00
+++ b/mysql-test/r/func_group.result	2007-12-21 18:24:00 +02:00
@@ -1407,4 +1407,13 @@ SELECT COUNT(*), a FROM t1;
 COUNT(*)	a
 4	1
 DROP TABLE t1;
+CREATE TABLE t1 (a INT, b INT);
+INSERT INTO t1 VALUES (1, 1);
+SELECT
+IF (1, 1/9, 0) * SUM(b) / 2 AS "correct",
+IF (a, 1/9, 0) * SUM(b) / 2 AS "wrong"
+FROM t1;
+correct	wrong
+0.05555556	0.05555556
+DROP TABLE t1;
 End of 5.0 tests
diff -Nrup a/mysql-test/t/func_group.test b/mysql-test/t/func_group.test
--- a/mysql-test/t/func_group.test	2007-11-01 18:36:24 +02:00
+++ b/mysql-test/t/func_group.test	2007-12-21 18:24:01 +02:00
@@ -901,5 +901,21 @@ SELECT COUNT(*), a FROM t1;
 
 DROP TABLE t1;
 
+#
+# Bug #31794: IF( 1, A, B ) * expr != IF( <true>, A, B ) * expr. --> 
+# Heavy rounding errors
+#
+
+CREATE TABLE t1 (a INT, b INT);
+
+INSERT INTO t1 VALUES (1, 1);
+
+SELECT
+IF (1, 1/9, 0) * SUM(b) / 2 AS "correct",
+IF (a, 1/9, 0) * SUM(b) / 2 AS "wrong"
+FROM t1;
+
+DROP TABLE t1;
+
 ###
 --echo End of 5.0 tests
diff -Nrup a/sql/item.cc b/sql/item.cc
--- a/sql/item.cc	2007-12-19 16:22:17 +02:00
+++ b/sql/item.cc	2007-12-21 18:24:01 +02:00
@@ -3048,38 +3048,6 @@ void Item_param::print(String *str)
 }
 
 
-/****************************************************************************
-  Item_copy_string
-****************************************************************************/
-
-void Item_copy_string::copy()
-{
-  String *res=item->val_str(&str_value);
-  if (res && res != &str_value)
-    str_value.copy(*res);
-  null_value=item->null_value;
-}
-
-/* ARGSUSED */
-String *Item_copy_string::val_str(String *str)
-{
-  // Item_copy_string is used without fix_fields call
-  if (null_value)
-    return (String*) 0;
-  return &str_value;
-}
-
-
-my_decimal *Item_copy_string::val_decimal(my_decimal *decimal_value)
-{
-  // Item_copy_string is used without fix_fields call
-  if (null_value)
-    return 0;
-  string2my_decimal(E_DEC_FATAL_ERROR, &str_value, decimal_value);
-  return (decimal_value);
-}
-
-
 /*
   Functions to convert item to field (for send_fields)
 */
diff -Nrup a/sql/item.h b/sql/item.h
--- a/sql/item.h	2007-12-13 12:49:11 +02:00
+++ b/sql/item.h	2007-12-21 18:24:01 +02:00
@@ -448,7 +448,7 @@ public:
 
   enum Type {FIELD_ITEM= 0, FUNC_ITEM, SUM_FUNC_ITEM, STRING_ITEM,
 	     INT_ITEM, REAL_ITEM, NULL_ITEM, VARBIN_ITEM,
-	     COPY_STR_ITEM, FIELD_AVG_ITEM, DEFAULT_VALUE_ITEM,
+	     FIELD_AVG_ITEM, DEFAULT_VALUE_ITEM,
 	     PROC_ITEM,COND_ITEM, REF_ITEM, FIELD_STD_ITEM,
 	     FIELD_VARIANCE_ITEM, INSERT_VALUE_ITEM,
              SUBSELECT_ITEM, ROW_ITEM, CACHE_ITEM, TYPE_HOLDER,
@@ -2178,51 +2178,6 @@ public:
 #include "item_uniq.h"
 #include "item_subselect.h"
 
-class Item_copy_string :public Item
-{
-  enum enum_field_types cached_field_type;
-public:
-  Item *item;
-  Item_copy_string(Item *i) :item(i)
-  {
-    null_value=maybe_null=item->maybe_null;
-    decimals=item->decimals;
-    max_length=item->max_length;
-    name=item->name;
-    cached_field_type= item->field_type();
-  }
-  enum Type type() const { return COPY_STR_ITEM; }
-  enum Item_result result_type () const { return STRING_RESULT; }
-  enum_field_types field_type() const { return cached_field_type; }
-  double val_real()
-  {
-    int err_not_used;
-    char *end_not_used;
-    return (null_value ? 0.0 :
-	    my_strntod(str_value.charset(), (char*) str_value.ptr(),
-		       str_value.length(), &end_not_used, &err_not_used));
-  }
-  longlong val_int()
-  {
-    int err;
-    return null_value ? LL(0) : my_strntoll(str_value.charset(),str_value.ptr(),
-                                            str_value.length(),10, (char**) 0,
-                                            &err); 
-  }
-  String *val_str(String*);
-  my_decimal *val_decimal(my_decimal *);
-  void make_field(Send_field *field) { item->make_field(field); }
-  void copy();
-  int save_in_field(Field *field, bool no_conversions)
-  {
-    return save_str_value_in_field(field, &str_value);
-  }
-  table_map used_tables() const { return (table_map) 1L; }
-  bool const_item() const { return 0; }
-  bool is_null() { return null_value; }
-};
-
-
 class Cached_item :public Sql_alloc
 {
 public:
@@ -2496,6 +2451,23 @@ public:
   bool eq(const Item *item, bool binary_cmp) const
   {
     return this == item;
+  }
+
+  void copy()
+  {
+    DBUG_ASSERT(example);
+    store(example);
+  }
+
+  Item *original_item() { return example; };
+  void make_field(Send_field *field) 
+  {
+    DBUG_ASSERT(example); 
+    example->make_field(field); 
+  }
+  enum_field_types field_type() const 
+  { 
+    return example ? example->field_type() : Item::field_type();  
   }
 };
 
diff -Nrup a/sql/sql_select.cc b/sql/sql_select.cc
--- a/sql/sql_select.cc	2007-12-20 12:23:59 +02:00
+++ b/sql/sql_select.cc	2007-12-21 18:24:02 +02:00
@@ -13125,9 +13125,9 @@ SORT_FIELD *make_unireg_sortorder(ORDER 
       pos->field= ((Item_field*) item)->field;
     else if (item->type() == Item::SUM_FUNC_ITEM && !item->const_item())
       pos->field= ((Item_sum*) item)->get_tmp_table_field();
-    else if (item->type() == Item::COPY_STR_ITEM)
+    else if (item->type() == Item::CACHE_ITEM)
     {						// Blob patch
-      pos->item= ((Item_copy_string*) item)->item;
+      pos->item= ((Item_cache*) item)->original_item();
     }
     else
       pos->item= *order->item;
@@ -14162,14 +14162,16 @@ setup_copy_fields(THD *thd, TMP_TABLE_PA
       pos= item;
       if (item->field->flags & BLOB_FLAG)
       {
-	if (!(pos= new Item_copy_string(pos)))
+	if (!(pos= Item_cache::get_cache(pos)))
 	  goto err;
+        if (((Item_cache *)pos)->setup(item))
+          goto err;
        /*
-         Item_copy_string::copy for function can call 
-         Item_copy_string::val_int for blob via Item_ref.
-         But if Item_copy_string::copy for blob isn't called before,
+         Item_cache::copy for function can call 
+         Item_cache::val_int for blob via Item_ref.
+         But if Item_cache::copy for blob isn't called before,
          it's value will be wrong
-         so let's insert Item_copy_string for blobs in the beginning of 
+         so let's insert Item_cache for blobs in the beginning of 
          copy_funcs
          (to see full test case look at having.test, BUG #4358) 
        */
@@ -14210,14 +14212,10 @@ setup_copy_fields(THD *thd, TMP_TABLE_PA
 	     !real_pos->with_sum_func)
     {						// Save for send fields
       pos= real_pos;
-      /* TODO:
-	 In most cases this result will be sent to the user.
-	 This should be changed to use copy_int or copy_real depending
-	 on how the value is to be used: In some cases this may be an
-	 argument in a group function, like: IF(ISNULL(col),0,COUNT(*))
-      */
-      if (!(pos=new Item_copy_string(pos)))
+      if (!(pos=Item_cache::get_cache(pos)))
 	goto err;
+      if (((Item_cache *)pos)->setup(real_pos))
+        goto err;
       if (i < border)                           // HAVING, ORDER and GROUP BY
       {
         if (extra_funcs.push_back(pos))
@@ -14269,8 +14267,8 @@ copy_fields(TMP_TABLE_PARAM *param)
     (*ptr->do_copy)(ptr);
 
   List_iterator_fast<Item> it(param->copy_funcs);
-  Item_copy_string *item;
-  while ((item = (Item_copy_string*) it++))
+  Item_cache *item;
+  while ((item = (Item_cache*) it++))
     item->copy();
 }
 
Thread
bk commit into 5.0 tree (gkodinov:1.2594) BUG#16854kgeorge21 Dec