Merge lp:~thisfred/u1db/iterate-over-list-of-dicts into lp:u1db

[Eric Casteleijn]

Added support for indexing dictionaries in lists, so this becomes possible:

        doc = self.db.create_doc_from_json(
            '{"foo": [{"zap": [{"qux": "fnord"}, {"qux": "zombo"}]}]}')
        self.db.create_index('test-idx', 'foo.zap.qux')
        self.assertEqual([doc], self.db.get_from_index('test-idx', 'fnord'))
        self.assertEqual([doc], self.db.get_from_index('test-idx', 'zombo'))

Added support for indexing dictionaries in lists, so this becomes possible:

        doc = self.db.create_doc_from_json(
            '{"foo": [{"zap": [{"qux": "fnord"}, {"qux": "zombo"}]}]}')
        self.db.create_index('test-idx', 'foo.zap.qux')
        self.assertEqual([doc], self.db.get_from_index('test-idx', 'fnord'))
        self.assertEqual([doc], self.db.get_from_index('test-idx', 'zombo'))

[Samuele Pedroni (pedronis)]

 + subfields = self.field.split('.')

I would move this to the constructor

255 + return extract_field(raw_doc, subfields)

+def extract_field(raw_doc, subfields):
206 + if not isinstance(raw_doc, dict):
207 + return []
208 + val = raw_doc.get(subfields.pop(0))

I would use a index argument and avoid the pop (especially pop(0)) and copy

216 + subfields = []
this line is not needed

same for these:
223 + if val is None:
224 + return []

line 209-210 cover that

[lp:~thisfred/u1db/iterate-over-list-of-dicts]

368. By Eric Casteleijn on 2012-07-31

split in __init__

369. By Eric Casteleijn on 2012-07-31

split in __init__

370. By Eric Casteleijn on 2012-07-31

don't mutate list

[Eric Casteleijn (thisfred)]

Fixes pushed

[lp:~thisfred/u1db/iterate-over-list-of-dicts]

371. By Eric Casteleijn on 2012-07-31

don't copy list around

372. By Eric Casteleijn on 2012-07-31

don't copy list around

[Samuele Pedroni (pedronis)]

+1

review: Approve

=== modified file 'src/u1db_query.c'
--- src/u1db_query.c	2012-07-27 15:17:04 +0000
+++ src/u1db_query.c	2012-07-31 14:58:20 +0000
@@ -252,60 +252,84 @@
     op_combine, "combine", json_type_string, -1, JUST_EXPRESSION};
 
 static int
-extract_field_values(json_object *obj, const string_list *field_path,
-                     int value_type, string_list *values)
+extract_value(json_object *val, int value_type, string_list *values)
 {
-    string_list_item *item = NULL;
+    int status = U1DB_OK;
+    int i, integer_value, boolean_value, length;
     char string_value[MAX_INT_STR_LEN];
-    struct array_list *list_val = NULL;
-    json_object *val = NULL;
-    int i, integer_value, boolean_value;
-    int status = U1DB_OK;
-    val = obj;
-    if (val == NULL)
-        goto finish;
-    for (item = field_path->head; item != NULL; item = item->next)
-    {
-        val = json_object_object_get(val, item->data);
-        if (val == NULL)
-            goto finish;
-    }
     if (json_object_is_type(val, json_type_string) && value_type ==
             json_type_string) {
-        if ((status = append(values, json_object_get_string(val))) != U1DB_OK)
-            goto finish;
-    } else if (json_object_is_type(val, json_type_int) && value_type ==
+        status = append(values, json_object_get_string(val));
+        goto finish;
+    }
+    if (json_object_is_type(val, json_type_int) && value_type ==
             json_type_int) {
         integer_value = json_object_get_int(val);
         snprintf(string_value, MAX_INT_STR_LEN, "%d", integer_value);
-        if ((status = append(values, string_value)) != U1DB_OK)
-            goto finish;
-    } else if (json_object_is_type(val, json_type_boolean) &&
-               value_type == json_type_boolean) {
+        status = append(values, string_value);
+        goto finish;
+    }
+    if (json_object_is_type(val, json_type_boolean) &&
+            value_type == json_type_boolean) {
         boolean_value = json_object_get_boolean(val);
         if (boolean_value) {
             status = append(values, "1");
-            if (status != U1DB_OK)
-                goto finish;
         } else {
             status = append(values, "0");
+        }
+        goto finish;
+    }
+    if (json_object_is_type(val, json_type_array)) {
+        length = json_object_array_length(val);
+        for (i = 0; i < length; i++) {
+            status = extract_value(
+                json_object_array_get_idx(val, i), value_type, values);
             if (status != U1DB_OK)
                 goto finish;
         }
-    } else if (json_object_is_type(val, json_type_array)) {
-        // TODO: recursively check the types
-        list_val = json_object_get_array(val);
-        for (i = 0; i < list_val->length; i++)
-        {
-            if ((status = append(values, json_object_get_string(
-                                array_list_get_idx(
-                                    list_val, i)))) != U1DB_OK)
-                goto finish;
-        }
-    }
-finish:
-    return status;
-}
+    }
+finish:
+    return status;
+}
+
+static int
+extract_field_values(json_object *obj, const string_list_item *field,
+                     int value_type, string_list *values)
+{
+    json_object *val = NULL;
+    json_object *array_item = NULL;
+    int i, length;
+    int status = U1DB_OK;
+
+    if (!json_object_is_type(obj, json_type_object)) {
+        goto finish;
+    }
+    val = json_object_object_get(obj, field->data);
+    if (val == NULL)
+        goto finish;
+    if (field->next != NULL) {
+        if (json_object_is_type(val, json_type_array)) {
+            length = json_object_array_length(val);
+            for (i = 0; i < length; i++) {
+                array_item = json_object_array_get_idx(val, i);
+                status = extract_field_values(
+                    array_item, field->next, value_type, values);
+                if (status != U1DB_OK)
+                    goto finish;
+            }
+            goto finish;
+        }
+        if (json_object_is_type(val, json_type_object)) {
+            status = extract_field_values(val, field->next, value_type, values);
+            goto finish;
+        }
+        goto finish;
+    }
+    status = extract_value(val, value_type, values);
+finish:
+    return status;
+}
+
 
 static int
 split(string_list *result, char *string, char splitter)
@@ -351,7 +375,7 @@
         status = ((op_function)tree->op)(tree, obj, values);
     } else {
         status = extract_field_values(
-            obj, tree->field_path, json_type_string, values);
+            obj, tree->field_path->head, json_type_string, values);
     }
     return status;
 }
@@ -411,7 +435,7 @@
     if (status != U1DB_OK)
         return status;
     status = extract_field_values(
-        obj, node->field_path, json_type_int, values);
+        obj, node->field_path->head, json_type_int, values);
     if (status != U1DB_OK)
         goto finish;
     node = node->next_sibling;
@@ -539,7 +563,7 @@
     //booleans by extract_field_values.
 
     status = extract_field_values(
-        obj, tree->first_child->field_path, json_type_boolean, values);
+        obj, tree->first_child->field_path->head, json_type_boolean, values);
     if (status != U1DB_OK)
         goto finish;
     for (item = values->head; item != NULL; item = item->next)
@@ -1274,7 +1298,7 @@
     char *sep = NULL;
     parse_tree *node = NULL;
     int status = U1DB_OK;
-    int i;
+    int i, array_size;
 
     sep = get_token(tokens);
     if (sep == NULL || strcmp(sep, "(") != 0) {
@@ -1321,8 +1345,9 @@
         }
     }
     i = 0;
+    array_size = sizeof(result->value_types) / sizeof(int);
     for (node = result->first_child; node != NULL; node = node->next_sibling) {
-        node->arg_type = result->value_types[i % result->number_of_children];
+        node->arg_type = result->value_types[i % array_size];
         if (node->arg_type == EXPRESSION) {
             status = to_getter(node);
             if (status != U1DB_OK)
@@ -1538,7 +1563,8 @@
     if (ctx->obj == NULL || !json_object_is_type(ctx->obj, json_type_object)) {
         return U1DB_INVALID_JSON;
     }
-    if ((status = init_list(&values)) != U1DB_OK)
+    status = init_list(&values);
+    if (status != U1DB_OK)
         goto finish;
     status = get_values(tree, ctx->obj, values);
     if (status != U1DB_OK)
@@ -1550,8 +1576,10 @@
             goto finish;
     }
 finish:
-    if (values != NULL)
+    if (values != NULL) {
         destroy_list(values);
+        values = NULL;
+    }
     return status;
 }
 

=== modified file 'u1db/query_parser.py'
--- u1db/query_parser.py	2012-07-27 14:50:11 +0000
+++ u1db/query_parser.py	2012-07-31 14:58:20 +0000
@@ -53,6 +53,29 @@
         return self.value
 
 
+def extract_field(raw_doc, subfields, index=0):
+    if not isinstance(raw_doc, dict):
+        return []
+    val = raw_doc.get(subfields[index])
+    if val is None:
+        return []
+    if index < len(subfields) - 1:
+        if isinstance(val, list):
+            results = []
+            for item in val:
+                results.extend(extract_field(item, subfields, index + 1))
+            return results
+        if isinstance(val, dict):
+            return extract_field(val, subfields, index + 1)
+        return []
+    if isinstance(val, dict):
+        return []
+    if isinstance(val, list):
+        # Strip anything in the list that isn't a simple type
+        return [v for v in val if not isinstance(v, (dict, list))]
+    return [val]
+
+
 class ExtractField(Getter):
     """Extract a field from the document."""
 
@@ -69,25 +92,10 @@
         :param field: a specifier for the field to return.
             This is either a field name, or a dotted field name.
         """
-        self.field = field
+        self.field = field.split('.')
 
     def get(self, raw_doc):
-        for subfield in self.field.split('.'):
-            if isinstance(raw_doc, dict):
-                raw_doc = raw_doc.get(subfield)
-            else:
-                return []
-        if isinstance(raw_doc, dict):
-            return []
-        if raw_doc is None:
-            result = []
-        elif isinstance(raw_doc, list):
-            # Strip anything in the list that isn't a simple type
-            result = [val for val in raw_doc
-                      if not isinstance(val, (dict, list))]
-        else:
-            result = [raw_doc]
-        return result
+        return extract_field(raw_doc, self.field)
 
 
 class Transformation(Getter):

=== modified file 'u1db/tests/test_backends.py'
--- u1db/tests/test_backends.py	2012-07-27 15:24:48 +0000
+++ u1db/tests/test_backends.py	2012-07-31 14:58:20 +0000
@@ -1470,6 +1470,26 @@
         self.db.create_index('test-idx', 'sub.foo.bar.baz.qux.fnord')
         self.assertEqual([], self.db.get_from_index('test-idx', '*'))
 
+    def test_nested_traverses_lists(self):
+        # subpath finds dicts in list
+        doc = self.db.create_doc_from_json(
+            '{"foo": [{"zap": "bar"}, {"zap": "baz"}]}')
+        # subpath only finds dicts in list
+        self.db.create_doc_from_json('{"foo": ["zap", "baz"]}')
+        self.db.create_index('test-idx', 'foo.zap')
+        self.assertEqual([doc], self.db.get_from_index('test-idx', 'bar'))
+        self.assertEqual([doc], self.db.get_from_index('test-idx', 'baz'))
+
+    def test_nested_list_traversal(self):
+        # subpath finds dicts in list
+        doc = self.db.create_doc_from_json(
+            '{"foo": [{"zap": [{"qux": "fnord"}, {"qux": "zombo"}]},'
+            '{"zap": "baz"}]}')
+        # subpath only finds dicts in list
+        self.db.create_index('test-idx', 'foo.zap.qux')
+        self.assertEqual([doc], self.db.get_from_index('test-idx', 'fnord'))
+        self.assertEqual([doc], self.db.get_from_index('test-idx', 'zombo'))
+
     def test_index_list1(self):
         self.db.create_index("index", "name")
         content = '{"name": ["foo", "bar"]}'

=== modified file 'u1db/tests/test_query_parser.py'
--- u1db/tests/test_query_parser.py	2012-07-26 13:35:50 +0000
+++ u1db/tests/test_query_parser.py	2012-07-31 14:58:20 +0000
@@ -179,6 +179,11 @@
     def test_get_value_list_of_dicts(self):
         self.assertExtractField([], 'foo', {'foo': [{'zap': 'bar'}]})
 
+    def test_get_value_list_of_dicts2(self):
+        self.assertExtractField(
+            ['bar', 'baz'], 'foo.zap',
+            {'foo': [{'zap': 'bar'}, {'zap': 'baz'}]})
+
     def test_get_value_int(self):
         self.assertExtractField([9], 'foo', {'foo': 9})
 
@@ -395,12 +400,12 @@
     def test_parse_field(self):
         getter = self.parse("a")
         self.assertIsInstance(getter, query_parser.ExtractField)
-        self.assertEqual("a", getter.field)
+        self.assertEqual(["a"], getter.field)
 
     def test_parse_dotted_field(self):
         getter = self.parse("a.b")
         self.assertIsInstance(getter, query_parser.ExtractField)
-        self.assertEqual("a.b", getter.field)
+        self.assertEqual(["a", "b"], getter.field)
 
     def test_parse_dotted_field_nothing_after_dot(self):
         self.assertParseError("a.")
@@ -427,12 +432,12 @@
         getter = self.parse("lower(a)")
         self.assertIsInstance(getter, query_parser.Lower)
         self.assertIsInstance(getter.inner, query_parser.ExtractField)
-        self.assertEqual("a", getter.inner.field)
+        self.assertEqual(["a"], getter.inner.field)
 
     def test_parse_all(self):
         getters = self.parse_all(["a", "b"])
         self.assertEqual(2, len(getters))
         self.assertIsInstance(getters[0], query_parser.ExtractField)
-        self.assertEqual("a", getters[0].field)
+        self.assertEqual(["a"], getters[0].field)
         self.assertIsInstance(getters[1], query_parser.ExtractField)
-        self.assertEqual("b", getters[1].field)
+        self.assertEqual(["b"], getters[1].field)

=== modified file 'u1db/tests/test_sqlite_backend.py'
--- u1db/tests/test_sqlite_backend.py	2012-07-19 19:50:58 +0000
+++ u1db/tests/test_sqlite_backend.py	2012-07-31 14:58:20 +0000
@@ -123,7 +123,7 @@
         self.db = sqlite_backend.SQLitePartialExpandDatabase(':memory:')
         g = self.db._parse_index_definition('fieldname')
         self.assertIsInstance(g, query_parser.ExtractField)
-        self.assertEqual('fieldname', g.field)
+        self.assertEqual(['fieldname'], g.field)
 
     def test__update_indexes(self):
         self.db = sqlite_backend.SQLitePartialExpandDatabase(':memory:')