Small fixes and added comments to condition pushdown to ndbcluster, after code review

sql/ha_ndbcluster.cc

@@ -5626,7 +5626,7 @@ extern "C" pthread_handler_decl(ndb_util_thread_func,
 
     /* Iterate through the  open files list */
     List_iterator_fast<NDB_SHARE> it(util_open_tables);
-    while (share= it++)
+    while ((share= it++))
     {
       /* Split tab- and dbname */
       char buf[FN_REFLEN];
@@ -5677,6 +5677,23 @@ extern "C" pthread_handler_decl(ndb_util_thread_func,
 /*
   Condition pushdown
 */
+/*
+  Push a condition to ndbcluster storage engine for evaluation 
+  during table   and index scans. The conditions will be stored on a stack
+  for possibly storing several conditions. The stack can be popped
+  by calling cond_pop, handler::extra(HA_EXTRA_RESET) (handler::reset())
+  will clear the stack.
+  The current implementation supports arbitrary AND/OR nested conditions
+  with comparisons between columns and constants (including constant
+  expressions and function calls) and the following comparison operators:
+  =, !=, >, >=, <, <=, "is null", and "is not null".
+  
+  RETURN
+    NULL The condition was supported and will be evaluated for each 
+    row found during the scan
+    cond The condition was not supported and all rows will be returned from
+         the scan for evaluation (and thus not saved on stack)
+*/
 const 
 COND* 
 ha_ndbcluster::cond_push(const COND *cond) 
@@ -5702,7 +5719,9 @@ ha_ndbcluster::cond_push(const COND *cond)
   DBUG_RETURN(cond); 
 }
 
-inline
+/*
+  Pop the top condition from the condition stack of the handler instance.
+*/
 void 
 ha_ndbcluster::cond_pop() 
 { 
@@ -5714,6 +5733,9 @@ ha_ndbcluster::cond_pop()
   }
 }
 
+/*
+  Clear the condition stack
+*/
 void
 ha_ndbcluster::cond_clear()
 {
@@ -5724,6 +5746,12 @@ ha_ndbcluster::cond_clear()
   DBUG_VOID_RETURN;
 }
 
+/*
+  Serialize the item tree into a linked list represented by Ndb_cond
+  for fast generation of NbdScanFilter. Adds information such as
+  position of fields that is not directly available in the Item tree.
+  Also checks if condition is supported.
+*/
 void ndb_serialize_cond(const Item *item, void *arg)
 {
   Ndb_cond_traverse_context *context= (Ndb_cond_traverse_context *) arg;
@@ -5747,14 +5775,14 @@ void ndb_serialize_cond(const Item *item, void *arg)
     case(Item::DECIMAL_ITEM):
       break;
     default:
-      *context->supported_ptr= FALSE;
+      context->supported= FALSE;
       break;
     }
 
     DBUG_VOID_RETURN;
   }
   
-  if (*context->supported_ptr)
+  if (context->supported)
   {
     Ndb_cond_stack *ndb_stack= context->stack_ptr;
     Ndb_cond *prev_cond= context->cond_ptr;
@@ -5844,7 +5872,7 @@ void ndb_serialize_cond(const Item *item, void *arg)
             break;
           }
         }
-	*context->supported_ptr= FALSE;
+        context->supported= FALSE;
         break;
       }
       case(Item::FUNC_ITEM): {
@@ -5946,14 +5974,14 @@ void ndb_serialize_cond(const Item *item, void *arg)
           DBUG_PRINT("info", ("LIKE_FUNC"));      
           curr_cond->ndb_item= new Ndb_item(func_item->functype());      
           context->expect(Item::STRING_ITEM);
-	  *context->supported_ptr= FALSE; // Currently not supported
+          context->supported= FALSE; // Currently not supported
           break;
         }
         case(Item_func::NOTLIKE_FUNC): {
           DBUG_PRINT("info", ("NOTLIKE_FUNC"));      
           curr_cond->ndb_item= new Ndb_item(func_item->functype());      
           context->expect(Item::STRING_ITEM);
-	  *context->supported_ptr= FALSE; // Currently not supported
+          context->supported= FALSE; // Currently not supported
           break;
         }
         case(Item_func::ISNULL_FUNC): {
@@ -6059,13 +6087,13 @@ void ndb_serialize_cond(const Item *item, void *arg)
             }
           else
             // Function does not return constant expression
-	    *context->supported_ptr= FALSE;
+            context->supported= FALSE;
           break;
         }
         default: {
           DBUG_PRINT("info", ("Found func_item of type %d", 
                               func_item->functype()));
-	  *context->supported_ptr= FALSE;
+          context->supported= FALSE;
         }
         }
         break;
@@ -6074,12 +6102,14 @@ void ndb_serialize_cond(const Item *item, void *arg)
         DBUG_PRINT("info", ("STRING_ITEM")); 
         if (context->expecting(Item::STRING_ITEM)) 
         {
+#ifndef DBUG_OFF
           char buff[256];
           String str(buff,(uint32) sizeof(buff), system_charset_info);
           str.length(0);
           Item_string *string_item= (Item_string *) item;
           DBUG_PRINT("info", ("value \"%s\"", 
                               string_item->val_str(&str)->ptr()));
+#endif
           NDB_ITEM_QUALIFICATION q;
           q.value_type= Item::STRING_ITEM;
           curr_cond->ndb_item= new Ndb_item(NDB_VALUE, q, item);      
@@ -6093,7 +6123,7 @@ void ndb_serialize_cond(const Item *item, void *arg)
             context->expect_nothing();
         }
         else
-	  *context->supported_ptr= FALSE;
+          context->supported= FALSE;
         break;
       case(Item::INT_ITEM): 
         DBUG_PRINT("info", ("INT_ITEM"));
@@ -6114,7 +6144,7 @@ void ndb_serialize_cond(const Item *item, void *arg)
             context->expect_nothing();
         }
         else
-	  *context->supported_ptr= FALSE;
+          context->supported= FALSE;
         break;
       case(Item::REAL_ITEM):
         DBUG_PRINT("info", ("REAL_ITEM %s"));
@@ -6135,18 +6165,20 @@ void ndb_serialize_cond(const Item *item, void *arg)
             context->expect_nothing();
         }
         else
-	  *context->supported_ptr= FALSE;
+          context->supported= FALSE;
         break;
       case(Item::VARBIN_ITEM):
         DBUG_PRINT("info", ("VARBIN_ITEM"));
         if (context->expecting(Item::VARBIN_ITEM)) 
         {
+#ifndef DBUG_OFF
           char buff[256];
           String str(buff,(uint32) sizeof(buff), system_charset_info);
           str.length(0);
           Item_hex_string *varbin_item= (Item_hex_string *) item;      
           DBUG_PRINT("info", ("value \"%s\"", 
                               varbin_item->val_str(&str)->ptr()));
+#endif
           NDB_ITEM_QUALIFICATION q;
           q.value_type= Item::VARBIN_ITEM;
           curr_cond->ndb_item= new Ndb_item(NDB_VALUE, q, item);      
@@ -6160,7 +6192,7 @@ void ndb_serialize_cond(const Item *item, void *arg)
             context->expect_nothing();
         }
         else
-	  *context->supported_ptr= FALSE;
+          context->supported= FALSE;
         break;
       case(Item::DECIMAL_ITEM):
         DBUG_PRINT("info", ("DECIMAL_ITEM %s"));
@@ -6182,7 +6214,7 @@ void ndb_serialize_cond(const Item *item, void *arg)
             context->expect_nothing();
         }
         else
-	  *context->supported_ptr= FALSE;
+          context->supported= FALSE;
         break;
       case(Item::COND_ITEM): {
         Item_cond *cond_item= (Item_cond *) item;
@@ -6197,14 +6229,14 @@ void ndb_serialize_cond(const Item *item, void *arg)
           break;
         default:
           DBUG_PRINT("info", ("COND_ITEM %d", cond_item->functype()));
-	  *context->supported_ptr= FALSE;
+          context->supported= FALSE;
           break;
         }
         break;
       }
       default: {
         DBUG_PRINT("info", ("Found item of type %d", item->type()));
-	*context->supported_ptr= FALSE;
+        context->supported= FALSE;
       }
       }
   }
@@ -6217,13 +6249,11 @@ ha_ndbcluster::serialize_cond(const COND *cond, Ndb_cond_stack *ndb_cond)
 {
   DBUG_ENTER("serialize_cond");
   Item *item= (Item *) cond;
-  bool supported= TRUE;
-  Ndb_cond_traverse_context context(table, (void *)m_table, 
-				    &supported, ndb_cond);
+  Ndb_cond_traverse_context context(table, (void *)m_table, ndb_cond);
   item->traverse_cond(&ndb_serialize_cond, (void *) &context, Item::PREFIX);
-  DBUG_PRINT("info", ("The pushed condition is %ssupported", (supported)?"":"not "));
+  DBUG_PRINT("info", ("The pushed condition is %ssupported", (context.supported)?"":"not "));
 
-  DBUG_RETURN(supported);
+  DBUG_RETURN(context.supported);
 }
 
 int
@@ -6485,7 +6515,8 @@ ha_ndbcluster::build_scan_filter_predicate(Ndb_cond * &cond,
       DBUG_RETURN(0);
     }
     case(Item_func::ISNULL_FUNC):
-      if (a->type == NDB_FIELD) {
+      if (a->type == NDB_FIELD) 
+      {
         DBUG_PRINT("info", ("Generating ISNULL filter"));
         if (filter->isnull(a->get_field_no()) == -1)
           DBUG_RETURN(1);
@@ -6493,7 +6524,8 @@ ha_ndbcluster::build_scan_filter_predicate(Ndb_cond * &cond,
       cond= cond->next->next;
       DBUG_RETURN(0);
     case(Item_func::ISNOTNULL_FUNC): {
-      if (a->type == NDB_FIELD) {
+      if (a->type == NDB_FIELD) 
+      {
         DBUG_PRINT("info", ("Generating ISNOTNULL filter"));
         if (filter->isnotnull(a->get_field_no()) == -1)
           DBUG_RETURN(1);         
@@ -6517,6 +6549,7 @@ int
 ha_ndbcluster::build_scan_filter_group(Ndb_cond* &cond, NdbScanFilter *filter)
 {
   DBUG_ENTER("build_scan_filter_group");
+  if (!cond) DBUG_RETURN(1);
   switch(cond->ndb_item->type) {
   case(NDB_FUNCTION):
     switch(cond->ndb_item->qualification.function_type) {

sql/ha_ndbcluster.h

@@ -87,6 +87,11 @@ typedef union ndb_item_value {
   NDB_ITEM_FIELD_VALUE *field_value;
 } NDB_ITEM_VALUE;
 
+/*
+  This class is used for serialization of the Item tree for
+  condition pushdown. It is stored in a linked list implemented
+  by Ndb_cond class.
+ */
 class Ndb_item {
  public:
   Ndb_item(NDB_ITEM_TYPE item_type) : type(item_type) {};
@@ -162,7 +167,12 @@ class Ndb_item {
   NDB_ITEM_VALUE value;
 };
 
-class Ndb_cond {
+/*
+  This class implements a linked list used for storing a
+  serialization of the Item tree for condition pushdown.
+ */
+class Ndb_cond 
+{
  public:
   Ndb_cond() : ndb_item(NULL), next(NULL), prev(NULL) {};
   ~Ndb_cond() 
@@ -177,7 +187,15 @@ class Ndb_cond {
   Ndb_cond *prev;
 };
 
-class Ndb_cond_stack {
+/*
+  This class implements a stack for storing several conditions
+  for pushdown (represented as serialized Item trees using Ndb_cond).
+  The current implementation only pushes one condition, but is
+  prepared for handling several (C1 AND C2 ...) if the logic for 
+  pushing conditions is extended in sql_select.
+*/
+class Ndb_cond_stack 
+{
  public:
   Ndb_cond_stack() : ndb_cond(NULL), next(NULL) {};
   ~Ndb_cond_stack() 
@@ -190,12 +208,19 @@ class Ndb_cond_stack {
   Ndb_cond_stack *next;
 };
 
-class Ndb_cond_traverse_context {
+/*
+  This class is used for storing the context when traversing
+  the Item tree. It stores a reference to the table the condition
+  is defined on, the serialized representation being generated, 
+  if the condition found is supported, and information what is
+  expected next in the tree inorder for the condition to be supported.
+*/
+class Ndb_cond_traverse_context 
+{
  public:
-  Ndb_cond_traverse_context(TABLE *tab, void* ndb_tab, 
-			    bool *supported, Ndb_cond_stack* stack)
+  Ndb_cond_traverse_context(TABLE *tab, void* ndb_tab, Ndb_cond_stack* stack)
     : table(tab), ndb_table(ndb_tab), 
-    supported_ptr(supported), stack_ptr(stack), cond_ptr(NULL),
+    supported(TRUE), stack_ptr(stack), cond_ptr(NULL),
     expect_mask(0), expect_field_result_mask(0), skip(0)
   {
     if (stack)
@@ -243,7 +268,7 @@ class Ndb_cond_traverse_context {
 
   TABLE* table;
   void* ndb_table;
-  bool *supported_ptr;
+  bool supported;
   Ndb_cond_stack* stack_ptr;
   Ndb_cond* cond_ptr;
   uint expect_mask;
@@ -255,7 +280,8 @@ class Ndb_cond_traverse_context {
   Place holder for ha_ndbcluster thread specific data
 */
 
-class Thd_ndb {
+class Thd_ndb 
+{
  public:
   Thd_ndb();
   ~Thd_ndb();
@@ -347,7 +373,27 @@ class ha_ndbcluster: public handler
   /*
     Condition pushdown
   */
+/*
+  Push a condition to ndbcluster storage engine for evaluation 
+  during table   and index scans. The conditions will be stored on a stack
+  for possibly storing several conditions. The stack can be popped
+  by calling cond_pop, handler::extra(HA_EXTRA_RESET) (handler::reset())
+  will clear the stack.
+  The current implementation supports arbitrary AND/OR nested conditions
+  with comparisons between columns and constants (including constant
+  expressions and function calls) and the following comparison operators:
+  =, !=, >, >=, <, <=, "is null", and "is not null".
+  
+  RETURN
+    NULL The condition was supported and will be evaluated for each 
+    row found during the scan
+    cond The condition was not supported and all rows will be returned from
+         the scan for evaluation (and thus not saved on stack)
+*/
   const COND *cond_push(const COND *cond);
+/*
+  Pop the top condition from the condition stack of the handler instance.
+*/
   void cond_pop();
 
   uint8 table_cache_type();

sql/handler.h

@@ -268,7 +268,7 @@ typedef struct st_table TABLE;
 struct st_foreign_key_info;
 typedef struct st_foreign_key_info FOREIGN_KEY_INFO;
 
-/* Forward declaration for Condition Pushdown to Handler (CPDH) */
+/* Forward declaration for condition pushdown to storage engine */
 typedef struct Item COND;
 
 typedef struct st_ha_check_opt
@@ -625,7 +625,24 @@ class handler :public Sql_alloc
  /*
    Condition pushdown to storage engines
  */
+
+/*
+  Push a condition to storage engine for evaluation during table
+  and index scans. The conditions should be stored on a stack
+  for possibly storing several conditions. The stack can be popped
+  by calling cond_pop, handler::extra(HA_EXTRA_RESET) (handler::reset())
+  should clear the stack.
+  The condition can be traversed using Item::traverse_cond
+  RETURN
+    NULL The condition was supported by the handler and will be evaluated
+         for each row found during the scan
+    cond The condition was not supported and all rows will be returned from
+         the scan for evaluation (and thus not saved on stack)
+*/
  virtual const COND *cond_push(const COND *cond) { return cond; };
+/*
+  Pop the top condition from the condition stack of the handler instance.
+*/
  virtual void cond_pop() { return; };
 };