fixed AQL optimiser bug, related to OR-comined conditions on the same attribute

CHANGELOG

@@ -1,5 +1,8 @@
 v1.1.beta3 (XXXX-XX-XX)
 
+* fixed AQL optimiser bug, related to OR-combined conditions that filtered on the
+  same attribute but with different conditions
+
 * fixed issue #277: allow usage of collection names when creating edges
   the fix of this issue also implies validation of collection names / ids passed to
   the REST edge create method. edges with invalid collection ids or names in the

arangod/Ahuacatl/ahuacatl-access-optimiser.c

@@ -27,8 +27,33 @@
 
 #include "ahuacatl-access-optimiser.h"
 
+#include "BasicsC/json.h"
+#include "BasicsC/string-buffer.h"
+
 #include "Ahuacatl/ahuacatl-functions.h"
 
+// -----------------------------------------------------------------------------
+// --SECTION--                                              forward declarations
+// -----------------------------------------------------------------------------
+
+static TRI_aql_attribute_name_t* GetAttributeName (TRI_aql_context_t* const,
+                                                   const TRI_aql_node_t* const);
+
+// -----------------------------------------------------------------------------
+// --SECTION--                                                     private types
+// -----------------------------------------------------------------------------
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief structure to contain stringified conditions, used for OR merging
+////////////////////////////////////////////////////////////////////////////////
+
+typedef struct or_element_s {
+  char*  _name;      // field name, e.g. "u.name"
+  char*  _condition; // stringified condition
+  size_t _count;     // number of times the same condition occurred
+}
+or_element_t;
+
 // -----------------------------------------------------------------------------
 // --SECTION--                                                 private functions
 // -----------------------------------------------------------------------------
@@ -67,6 +92,238 @@ static const char* TypeName (const TRI_aql_access_e type) {
   return "unknown";
 }
 
+////////////////////////////////////////////////////////////////////////////////
+/// @brief get the type name for a range access
+////////////////////////////////////////////////////////////////////////////////
+
+static const char* RangeName (const TRI_aql_range_e type) {
+  switch (type) {
+    case TRI_AQL_RANGE_LOWER_EXCLUDED:
+      return "lower (exc)";
+    case TRI_AQL_RANGE_LOWER_INCLUDED:
+      return "lower (inc)";
+    case TRI_AQL_RANGE_UPPER_EXCLUDED:
+      return "upper (exc)";
+    case TRI_AQL_RANGE_UPPER_INCLUDED:
+      return "upper (inc)";
+  }
+
+  return "unknown";
+}
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief stringify a json value
+////////////////////////////////////////////////////////////////////////////////
+
+static void StringifyFieldAccessJson (TRI_aql_context_t* const context,
+                                      const TRI_json_t* const json,
+                                      TRI_string_buffer_t* const buffer) {
+  TRI_StringifyJson(buffer, json);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief stringify a range access
+////////////////////////////////////////////////////////////////////////////////
+
+static void StringifyFieldAccessRange (TRI_aql_context_t* const context,
+                                       const TRI_aql_range_t* const range,
+                                       TRI_string_buffer_t* const buffer) {
+  TRI_AppendStringStringBuffer(buffer, RangeName(range->_type));
+  TRI_AppendCharStringBuffer(buffer, ':');
+  StringifyFieldAccessJson(context, range->_value, buffer);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief stringify a reference access
+////////////////////////////////////////////////////////////////////////////////
+
+static void StringifyFieldAccessReference (TRI_aql_context_t* const context,
+                                           const TRI_aql_field_access_t* const fieldAccess,
+                                           TRI_string_buffer_t* const buffer) {
+  TRI_AppendStringStringBuffer(buffer, TRI_NodeNameAql(fieldAccess->_value._reference._operator));
+  TRI_AppendCharStringBuffer(buffer, ':');
+
+  if (fieldAccess->_value._reference._type == TRI_AQL_REFERENCE_VARIABLE) {
+    TRI_AppendStringStringBuffer(buffer, "variable:");
+    // append variable name
+    TRI_AppendStringStringBuffer(buffer, fieldAccess->_value._reference._ref._name);
+  }
+  else if (fieldAccess->_value._reference._type == TRI_AQL_REFERENCE_ATTRIBUTE_ACCESS) {
+    TRI_aql_attribute_name_t* attributeName;
+
+    attributeName = GetAttributeName(context, fieldAccess->_value._reference._ref._node);
+    if (attributeName == NULL) {
+      // out of memory
+      return;
+    }
+
+    // append node info
+    TRI_AppendStringStringBuffer(buffer, "attribute:");
+    TRI_AppendStringStringBuffer(buffer, attributeName->_name._buffer);
+      
+    TRI_DestroyStringBuffer(&attributeName->_name);
+    TRI_Free(TRI_UNKNOWN_MEM_ZONE, attributeName);
+  }
+  else {
+    assert(false);
+  }
+}
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief stringify a field access
+////////////////////////////////////////////////////////////////////////////////
+
+static char* StringifyFieldAccess (TRI_aql_context_t* const context,
+                                   const TRI_aql_field_access_t* const fieldAccess) {
+  TRI_string_buffer_t buffer;
+  char* result;
+  
+  TRI_InitStringBuffer(&buffer, TRI_UNKNOWN_MEM_ZONE);
+  if (buffer._buffer == NULL) {
+    // out of memory
+    return NULL;
+  }
+
+  TRI_AppendStringStringBuffer(&buffer, TypeName(fieldAccess->_type));
+
+  switch (fieldAccess->_type) {
+    case TRI_AQL_ACCESS_EXACT:
+    case TRI_AQL_ACCESS_LIST:
+      TRI_AppendCharStringBuffer(&buffer, ':');
+      StringifyFieldAccessJson(context, fieldAccess->_value._value, &buffer);
+      break;
+    
+    case TRI_AQL_ACCESS_RANGE_SINGLE:
+      TRI_AppendCharStringBuffer(&buffer, ':');
+      StringifyFieldAccessRange(context, &fieldAccess->_value._singleRange, &buffer);
+      break;
+
+    case TRI_AQL_ACCESS_RANGE_DOUBLE:
+      TRI_AppendCharStringBuffer(&buffer, ':');
+      StringifyFieldAccessRange(context, &fieldAccess->_value._between._lower, &buffer);
+      TRI_AppendCharStringBuffer(&buffer, ':');
+      StringifyFieldAccessRange(context, &fieldAccess->_value._between._upper, &buffer);
+      break;
+    
+    case TRI_AQL_ACCESS_REFERENCE:
+      TRI_AppendCharStringBuffer(&buffer, ':');
+      StringifyFieldAccessReference(context, fieldAccess, &buffer);
+      break;
+
+    case TRI_AQL_ACCESS_IMPOSSIBLE:
+    case TRI_AQL_ACCESS_ALL: 
+    default: {
+      // nothing to do here
+    }
+  }
+  
+  result = TRI_DuplicateStringZ(TRI_UNKNOWN_MEM_ZONE, buffer._buffer);
+  TRI_DestroyStringBuffer(&buffer);
+
+  return result;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief get an or element by field name
+////////////////////////////////////////////////////////////////////////////////
+
+static or_element_t* FindOrElement (const TRI_vector_pointer_t* const vector,
+                                    const char* const name) {
+  size_t i, n;
+
+  assert(vector);
+  assert(name);
+
+  n = vector->_length;
+  for (i = 0; i < n; ++i) {
+    or_element_t* current;
+    
+    current = (or_element_t*) TRI_AtVectorPointer(vector, i);
+
+    assert(current);
+
+    if (TRI_EqualString(name, current->_name)) {
+      return current;
+    }
+  }
+
+  return NULL;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief insert all field accesses from a vector into an OR aggregation vector
+////////////////////////////////////////////////////////////////////////////////
+
+static bool InsertOrs (TRI_aql_context_t* const context,
+                       const TRI_vector_pointer_t* const source, 
+                       TRI_vector_pointer_t* const dest) {
+  size_t i, n;
+
+  if (dest == NULL) {
+    return true;
+  }
+
+  n = source->_length;
+  for (i = 0; i < n; ++i) {
+    or_element_t* orElement;
+    TRI_aql_field_access_t* fieldAccess;
+    
+    fieldAccess = (TRI_aql_field_access_t*) TRI_AtVectorPointer(source, i);
+
+    assert(fieldAccess);
+    assert(fieldAccess->_fullName);
+
+    orElement = FindOrElement(dest, fieldAccess->_fullName);
+    if (orElement == NULL) {
+      // element not found. create a new one
+      orElement = TRI_Allocate(TRI_UNKNOWN_MEM_ZONE, sizeof(or_element_t), false);
+      if (orElement == NULL) {
+        // out of memory
+        return false;
+      }
+
+      orElement->_count = 1;
+      orElement->_condition = StringifyFieldAccess(context, fieldAccess);
+
+      if (orElement->_condition == NULL) {
+        // out of memory
+        TRI_Free(TRI_UNKNOWN_MEM_ZONE, orElement);
+
+        return false;
+      }
+
+      orElement->_name = TRI_DuplicateStringZ(TRI_UNKNOWN_MEM_ZONE, fieldAccess->_fullName);
+      if (orElement->_name == NULL) {
+        // out of memory
+        TRI_FreeString(TRI_UNKNOWN_MEM_ZONE, orElement->_condition);
+        TRI_Free(TRI_UNKNOWN_MEM_ZONE, orElement);
+
+        return false;
+      }
+
+      TRI_PushBackVectorPointer(dest, (void*) orElement);
+    }
+    else {
+      // compare previous with current condition
+      char* condition = StringifyFieldAccess(context, fieldAccess);
+
+      if (condition == NULL) {
+        // out of memory
+        return false;
+      }
+
+      if (TRI_EqualString(condition, orElement->_condition)) {
+        // conditions are identical, match!
+        orElement->_count++;
+      }
+
+      TRI_FreeString(TRI_UNKNOWN_MEM_ZONE, condition);
+    }
+  }
+
+  return true;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 /// @brief set the length of the variable name for a field access struct
 ////////////////////////////////////////////////////////////////////////////////
@@ -1722,56 +1979,6 @@ static void InsertVector (TRI_aql_context_t* const context,
   }
 }
 
-////////////////////////////////////////////////////////////////////////////////
-/// @brief count how often an element is contained in a string vector
-////////////////////////////////////////////////////////////////////////////////
-
-static size_t CountNames (const char* const name,
-                          const TRI_vector_pointer_t* const vector) {
-  size_t found;
-  size_t i, n;
-
-  assert(vector);
-  assert(name);
-
-  found = 0;
-  
-  n = vector->_length;
-  for (i = 0; i < n; ++i) {
-    char* current = (char*) TRI_AtVectorPointer(vector, i);
-
-    if (TRI_EqualString(name, current)) {
-      ++found;
-    }
-  }
-
-  return found;
-}
-
-////////////////////////////////////////////////////////////////////////////////
-/// @brief insert the names of all field accesses from a vector into a 
-/// string vector
-////////////////////////////////////////////////////////////////////////////////
-
-static void InsertNames (const TRI_vector_pointer_t* const source, 
-                         TRI_vector_pointer_t* const dest) {
-  size_t i, n;
-
-  if (dest == NULL) {
-    return;
-  }
-
-  n = source->_length;
-  for (i = 0; i < n; ++i) {
-    TRI_aql_field_access_t* fieldAccess = (TRI_aql_field_access_t*) TRI_AtVectorPointer(source, i);
-
-    char* copy = TRI_DuplicateStringZ(TRI_UNKNOWN_MEM_ZONE, fieldAccess->_fullName);
-    if (copy) {
-      TRI_PushBackVectorPointer(dest, (void*) copy);
-    }
-  }
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 /// @brief merge two attribute access vectors using logical AND or OR
 ////////////////////////////////////////////////////////////////////////////////
@@ -1782,7 +1989,7 @@ static TRI_vector_pointer_t* MergeVectors (TRI_aql_context_t* const context,
                                            TRI_vector_pointer_t* const rhs,
                                            const TRI_vector_pointer_t* const inheritedRestrictions) {
   TRI_vector_pointer_t* result;
-  TRI_vector_pointer_t* namesVector;
+  TRI_vector_pointer_t* orElements;
 
   assert(context);
   assert(mergeType == TRI_AQL_NODE_OPERATOR_BINARY_AND || mergeType == TRI_AQL_NODE_OPERATOR_BINARY_OR);
@@ -1818,26 +2025,31 @@ static TRI_vector_pointer_t* MergeVectors (TRI_aql_context_t* const context,
     return NULL;
   }
   
-  namesVector = NULL;
+  orElements = NULL;
 
   if (mergeType == TRI_AQL_NODE_OPERATOR_BINARY_OR &&  lhs && rhs) {
     // this is an OR merge
     // we need to check which elements are contained in just one of the vectors and
     // remove them. if we don't do this, we would probably restrict the query too much
-    namesVector = TRI_Allocate(TRI_UNKNOWN_MEM_ZONE, sizeof(TRI_vector_pointer_t), false);
+    orElements = TRI_Allocate(TRI_UNKNOWN_MEM_ZONE, sizeof(TRI_vector_pointer_t), false);
-    TRI_InitVectorPointer(namesVector, TRI_UNKNOWN_MEM_ZONE);
+    if (orElements == NULL) {
+      TRI_SetErrorContextAql(context, TRI_ERROR_OUT_OF_MEMORY, NULL);
+
+      return NULL;
+    }
+
+    TRI_InitVectorPointer(orElements, TRI_UNKNOWN_MEM_ZONE);
   }
   
 
   if (inheritedRestrictions) {
-    InsertNames(inheritedRestrictions, namesVector);
-
     // insert a copy of all restrictions first
     InsertVector(context, result, inheritedRestrictions);
   }
 
   if (lhs) {
-    InsertNames(lhs, namesVector);
+    // this is a no-op if there is no orElements vector
+    InsertOrs(context, lhs, orElements);
 
     // copy elements from lhs into result vector
     MergeVector(context, mergeType, result, lhs);
@@ -1845,7 +2057,8 @@ static TRI_vector_pointer_t* MergeVectors (TRI_aql_context_t* const context,
   }
 
   if (rhs) {
-    InsertNames(rhs, namesVector);
+    // this is a no-op if there is no orElements vector
+    InsertOrs(context, rhs, orElements);
 
     // copy elements from rhs into result vector
     MergeVector(context, mergeType, result, rhs);
@@ -1853,36 +2066,45 @@ static TRI_vector_pointer_t* MergeVectors (TRI_aql_context_t* const context,
   }
 
 
-  if (namesVector) {
+  if (orElements) {
     // this is an OR merge
     // we need to check which elements are contained in just one of the vectors and
     // remove them. if we don't do this, we would probably restrict the query too much
-    size_t found;
     size_t i;
 
     for (i = 0; i < result->_length; ++i) {
-      TRI_aql_field_access_t* fieldAccess = (TRI_aql_field_access_t*) TRI_AtVectorPointer(result, i);
+      TRI_aql_field_access_t* fieldAccess;
+      or_element_t* orElement;
+      
+      fieldAccess = (TRI_aql_field_access_t*) TRI_AtVectorPointer(result, i);
+      assert(fieldAccess);
 
       if (fieldAccess->_type == TRI_AQL_ACCESS_ALL) {
         continue;
       }
 
-      found = CountNames(fieldAccess->_fullName, namesVector);
+      orElement = FindOrElement(orElements, fieldAccess->_fullName);
-      
+      if (orElement == NULL || orElement->_count < 2) {
-      if (found < 2) {
         // must make the element an all access
         FreeAccessMembers(fieldAccess);
         fieldAccess->_type = TRI_AQL_ACCESS_ALL;
       }
     }
     
-    for (i = 0; i < namesVector->_length; ++i) {
+    // free all orElements
-      char* name = (char*) TRI_AtVectorPointer(namesVector, i); 
+    for (i = 0; i < orElements->_length; ++i) {
+      or_element_t* orElement;
 
-      TRI_Free(TRI_UNKNOWN_MEM_ZONE, name);
+      orElement = (or_element_t*) TRI_AtVectorPointer(orElements, i); 
+      assert(orElement);
+
+      // free members
+      TRI_FreeString(TRI_UNKNOWN_MEM_ZONE, orElement->_name);
+      TRI_FreeString(TRI_UNKNOWN_MEM_ZONE, orElement->_condition);
+      TRI_Free(TRI_UNKNOWN_MEM_ZONE, orElement);
     }
 
-    TRI_FreeVectorPointer(TRI_UNKNOWN_MEM_ZONE, namesVector);
+    TRI_FreeVectorPointer(TRI_UNKNOWN_MEM_ZONE, orElements);
   }
     
   return result;