Address review feedback

- One iterator class
- Switch back from state automaton into two-nested-iterators approach..

mysql-test/t/join_nested.test

@@ -462,7 +462,6 @@ SELECT t2.a,t2.b,t3.a,t3.b,t4.a,t4.b
        LEFT JOIN              
        (t1,t2)
        ON t3.a=1 AND t3.b=t2.b AND t2.b=t4.b;
-
 SELECT t2.a,t2.b,t3.a,t3.b,t4.a,t4.b
   FROM (t3,t4)
        LEFT JOIN              

mysql-test/t/subselect_sj2.test

@@ -586,6 +586,7 @@ if (`select @@join_cache_level=6`)
   --echo # Not anymore:
   --echo # The following query gives wrong result due to Bug#49129
 }
+select sin(0);
 select * from t0 where t0.a in 
   (select t1.a from t1, t2 where t2.a=t0.a and t1.b=t2.b);
 

sql/multi_range_read.h

@@ -48,6 +48,37 @@
 
 #include "sql_lifo_buffer.h"
 
+class DsMrr_impl;
+
+/**
+  Iterator over (record, range_id) pairs that match given key value.
+  
+  We may need to scan multiple (key_val, range_id) pairs with the same 
+  key value. A key value may have multiple matching records, so we'll need to
+  produce a cross-product of sets of matching records and range_id-s.
+*/
+
+class Key_value_records_iterator
+{
+  /* Scan parameters */
+  DsMrr_impl *dsmrr;
+  Lifo_buffer_iterator identical_key_it;
+  uchar *last_identical_key_ptr;
+  bool get_next_row;
+public:
+  /*
+  */
+  bool init(DsMrr_impl *dsmrr);
+
+  /*
+    Get next (key_val, range_id) pair.
+  */
+  int get_next();
+
+  void close();
+};
+
+
 /*
   DS-MRR implementation for one table. Create/use one object of this class for
   each ha_{myisam/innobase/etc} object. That object will be further referred to
@@ -196,17 +227,6 @@ private:
   uchar *rowid_buffer_end;
  
   /** Index scaning and key buffer-related members **/
-  
-
-  enum enum_index_scan_state {
-    REFILL_KEY_BUFFER,
-    GET_NEXT_RANGE,
-    GET_NEXT_RECORD,
-    GET_NEXT_IDENTICAL_KEY,
-    SCAN_FINISHED
-  };
-
-  enum enum_index_scan_state index_scan_state;
 
   /* TRUE <=> We can get at most one index tuple for a lookup key */
   bool index_ranges_unique;
@@ -220,25 +240,26 @@ private:
     buffers.
   */
   Forward_lifo_buffer forward_key_buf;
-  Forward_iterator forward_key_it;
   Backward_lifo_buffer backward_key_buf;
-  Backward_iterator backward_key_it;
 
   /* Buffer to store (key, range_id) pairs */
   Lifo_buffer *key_buffer;
-   
-  /* key_buffer.read() reads */
-  uchar *cur_index_tuple;
-
-  /* if in_index_range==TRUE: range_id of the range we're enumerating */
-  char *cur_range_info;
-
+  
+  /* Index scan state */
+  bool scanning_key_val_iter;
   /* 
     TRUE <=> we've got index tuples/rowids for all keys (need this flag because 
     we may have a situation where we've read everything from the key buffer but 
     haven't finished with getting index tuples for the last key)
   */
-  //bool key_eof;
+  bool index_scan_eof;  
+  Key_value_records_iterator kv_it;
+  
+  /* key_buffer.read() reads to here */
+  uchar *cur_index_tuple;
+
+  /* if in_index_range==TRUE: range_id of the range we're enumerating */
+  char *cur_range_info;
 
   /* Initially FALSE, becomes TRUE when we've set key_tuple_xxx members */
   bool know_key_tuple_params;
@@ -255,28 +276,6 @@ private:
   /* = key_size_in_keybuf [ + sizeof(range_assoc_info) ] */
   uint key_buff_elem_size;
   
-  /* 
-    TRUE <=> we're doing key-ordered index scan and right now several
-    subsequent key values are the same as the one we've already retrieved and
-    returned index tuple for.
-  */
-  //bool in_identical_keys_range;
-
-  /* range_id of the first of the identical keys */
-  char *first_identical_range_info;
-
-  /* Pointer to the last of the identical key values */
-  uchar *last_identical_key_ptr;
-
-  /* 
-    key_buffer iterator for walking the identical key range (we need to
-    enumerate the set of (identical_key, range_id) pairs multiple times,
-    and do that by walking from current buffer read position until we get
-    last_identical_key_ptr.
-  */
-  Lifo_buffer::Iterator *identical_key_it;
-
-
   /** rnd_pos() scan and rowid buffer-related members **/
 
   /*
@@ -288,12 +287,7 @@ private:
   /* rowid_buffer.read() will set the following:  */
   uchar *rowid;
   uchar *rowids_range_id;
-  
-  /*
-    not-NULL: we're traversing a group of (rowid, range_id) pairs with
-              identical rowid values, and this is the pointer to the last one.
-    NULL: we're not in the group of indentical rowids.
-  */
+
   uchar *last_identical_rowid;
 
   bool dsmrr_eof; /* TRUE <=> We have reached EOF when reading index tuples */
@@ -315,10 +309,9 @@ private:
   void setup_buffer_sizes(key_range *sample_key);
   void reallocate_buffer_space();
   
-  void read_out_identical_ranges();
-
   static range_seq_t key_buf_seq_init(void *init_param, uint n_ranges, uint flags);
   static uint key_buf_seq_next(range_seq_t rseq, KEY_MULTI_RANGE *range);
+  friend class Key_value_records_iterator;
 };
 
 /**

sql/sql_lifo_buffer.h

@@ -112,40 +112,17 @@ public:
   virtual void reset() = 0;
   virtual uchar *end_of_space() = 0;
 protected:
-  bool have_data(size_t bytes)
-  {
-    return (used_size() >= bytes);
-  }
   virtual bool have_space_for(size_t bytes) = 0;
   virtual size_t used_size() = 0;
-
+  
+  /* To be used only by iterator class: */
+  virtual uchar *get_pos()= 0;
+  virtual bool read(uchar **position)= 0;
+  friend class Lifo_buffer_iterator;
 public:
-
   virtual void remove_unused_space(uchar **unused_start, uchar **unused_end)=0;
-  virtual uchar *used_area() = 0;
-   
-  /** Iterator to walk over contents of the buffer without reading it. */
-  class Iterator
-  {
-  public:
-    virtual void init(Lifo_buffer *buf) = 0;
-    /*
-      Read the next value. The calling convention is the same as buf->read()
-      has.
-
-      @retval FALSE - ok
-      @retval TRUE  - EOF, reached the end of the buffer
-    */
-    virtual bool read_next()= 0;
-    virtual ~Iterator() {}
-  protected:
-    Lifo_buffer *buf;
-    virtual uchar *get_next(size_t nbytes)=0;
-  };
+  virtual uchar *used_area() = 0; 
   virtual ~Lifo_buffer() {};
-
-  friend class Forward_iterator;
-  friend class Backward_iterator;
 };
 
 
@@ -196,19 +173,24 @@ public:
     memcpy(pos, data, bytes);
     pos += bytes;
   }
-  uchar *read_bytes(size_t bytes)
+  bool have_data(uchar *position, size_t bytes)
   {
-    DBUG_ASSERT(have_data(bytes));
-    pos= pos - bytes;
-    return pos;
+    return ((position - start) >= (ptrdiff_t)bytes);
   }
-  bool read()
+  uchar *read_bytes(uchar **position, size_t bytes)
+  {
+    DBUG_ASSERT(have_data(*position, bytes));
+    *position= (*position) - bytes;
+    return *position;
+  }
+  bool read() { return read(&pos); }
+  bool read(uchar **position)
   {
-    if (!have_data(size1 + (read_ptr2 ? size2 : 0)))
+    if (!have_data(*position, size1 + (read_ptr2 ? size2 : 0)))
       return TRUE;
     if (read_ptr2)
-      *read_ptr2= read_bytes(size2);
-    *read_ptr1= read_bytes(size1);
+      *read_ptr2= read_bytes(position, size2);
+    *read_ptr1= read_bytes(position, size1);
     return FALSE;
   }
   void remove_unused_space(uchar **unused_start, uchar **unused_end)
@@ -231,58 +213,11 @@ public:
   }
   /* Return pointer to start of the memory area that is occupied by the data */
   uchar *used_area() { return start; }
-  friend class Forward_iterator;
+  friend class Lifo_buffer_iterator;
+  uchar *get_pos() { return pos; }
 };
 
 
-/**
-  Iterator for Forward_lifo_buffer
-*/
-
-class Forward_iterator : public Lifo_buffer::Iterator
-{
-  uchar *pos;
-
-  /** Return pointer to next chunk of nbytes bytes and avance over it */
-  uchar *get_next(size_t nbytes)
-  {
-    if (pos - nbytes < ((Forward_lifo_buffer*)buf)->start)
-      return NULL;
-    pos -= nbytes;
-    return pos;
-  }
-public:
-  bool read_next()
-  {
-    uchar *res;
-    if (buf->read_ptr2)
-    {
-      if ((res= get_next(buf->size2)))
-      {
-        *(buf->read_ptr2)= res;
-        *buf->read_ptr1= get_next(buf->size1);
-        return FALSE;
-      }
-    }
-    else
-    {
-      if ((res= get_next(buf->size1)))
-      {
-        *(buf->read_ptr1)= res;
-        return FALSE;
-      }
-    }
-    return TRUE; /* EOF */
-  }
-
-  void init(Lifo_buffer *buf_arg)
-  {
-    DBUG_ASSERT(buf_arg->type() == Lifo_buffer::FORWARD);
-    buf= buf_arg;
-    pos= ((Forward_lifo_buffer*)buf)->pos;
-  }
-};
-
 
 /**
   Backward LIFO buffer
@@ -332,18 +267,26 @@ public:
   }
   bool read()
   {
-    if (!have_data(size1 + (read_ptr2 ? size2 : 0)))
+    return read(&pos);
+  }
+  bool read(uchar **position)
+  {
+    if (!have_data(*position, size1 + (read_ptr2 ? size2 : 0)))
       return TRUE;
-    *read_ptr1= read_bytes(size1);
+    *read_ptr1= read_bytes(position, size1);
     if (read_ptr2)
-      *read_ptr2= read_bytes(size2);
+      *read_ptr2= read_bytes(position, size2);
     return FALSE;
   }
-  uchar *read_bytes(size_t bytes)
+  bool have_data(uchar *position, size_t bytes)
+  {
+    return ((end - position) >= (ptrdiff_t)bytes);
+  }
+  uchar *read_bytes(uchar **position, size_t bytes)
   {
-    DBUG_ASSERT(have_data(bytes));
-    uchar *ret= pos;
-    pos= pos + bytes;
+    DBUG_ASSERT(have_data(*position, bytes));
+    uchar *ret= *position;
+    *position= *position + bytes;
     return ret;
   }
   /**
@@ -363,50 +306,34 @@ public:
   }
   /* Return pointer to start of the memory area that is occupied by the data */
   uchar *used_area() { return pos; }
-  friend class Backward_iterator;
+  friend class Lifo_buffer_iterator;
+  uchar *get_pos() { return pos; }
 };
 
 
-/**
-  Iterator for Backward_lifo_buffer
-*/
 
-class Backward_iterator : public Lifo_buffer::Iterator
+/** Iterator to walk over contents of the buffer without reading it. */
+class Lifo_buffer_iterator
 {
   uchar *pos;
-  /* Return pointer to next chunk of nbytes bytes and advance over it */
-  uchar *get_next(size_t nbytes)
-  {
-    if (pos + nbytes > ((Backward_lifo_buffer*)buf)->end)
-      return NULL;
-    uchar *res= pos;
-    pos += nbytes;
-    return res;
-  }
+  Lifo_buffer *buf;
 public:
-  bool read_next()
-  {
-    /*
-      Always read the first component first (if the buffer is backwards, we
-      have written the second component first).
-    */
-    uchar *res;
-    if ((res= get_next(buf->size1)))
-    {
-      *(buf->read_ptr1)= res;
-      if (buf->read_ptr2)
-        *buf->read_ptr2= get_next(buf->size2);
-      return FALSE;
-    }
-    return TRUE; /* EOF */
-  }
   void init(Lifo_buffer *buf_arg)
   {
-    DBUG_ASSERT(buf_arg->type() == Lifo_buffer::BACKWARD);
     buf= buf_arg;
-    pos= ((Backward_lifo_buffer*)buf)->pos;
+    pos= buf->get_pos();
   }
-};
+  /*
+    Read the next value. The calling convention is the same as buf->read()
+    has.
 
+    @retval FALSE - ok
+    @retval TRUE  - EOF, reached the end of the buffer
+  */
+  bool read() 
+  {
+    return buf->read(&pos);
+  }
+};