Merge lp:~thisfred/u1db/combine-mapping-1 into lp:u1db

[Eric Casteleijn]

Changed the Python query parser to be a little more versatile. It now allows for arbitrary numbers and types of arguments, which combine() will need.

Changed the Python query parser to be a little more versatile. It now allows for arbitrary numbers and types of arguments, which combine() will need.

Next step: do the same for C, which will definitely be a more interesting challenge.

[lp:~thisfred/u1db/combine-mapping-1]

356. By Eric Casteleijn on 2012-07-17

better error msges

[Eric Casteleijn (thisfred)]

Rejecting this, in favor of newer branch.

Unmerged revisions

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2012-07-16 16:20:53 +0000
+++ CMakeLists.txt	2012-07-17 20:51:27 +0000
@@ -44,6 +44,11 @@
     COMMAND cd html-docs && make doctest && cd ..
 )
 
+add_custom_target(check-nodoc
+    COMMAND python -m testtools.run discover
+    DEPENDS build-inplace
+)
+
 add_custom_target(check
     COMMAND python -m testtools.run discover
     DEPENDS build-inplace doctests

=== modified file 'u1db/query_parser.py'
--- u1db/query_parser.py	2012-05-15 16:23:19 +0000
+++ u1db/query_parser.py	2012-07-17 20:51:27 +0000
@@ -21,9 +21,66 @@
     )
 
 
+def make_subtree(expression, start, idx, open_parens):
+    tree = []
+    while idx < len(expression):
+        char = expression[idx]
+        if char == '(':
+            open_parens.append(1)
+            term = expression[start:idx].strip()
+            if term:
+                tree.append(term)
+            else:
+                raise errors.IndexDefinitionParseError(
+                    "Missing operator name in: \n%s\n%s^" %
+                    (expression, " " * idx))
+            idx += 1
+            start = idx
+            idx, start, subtree = make_subtree(
+                expression, start, idx, open_parens)
+            tree.append(subtree)
+        elif char == ')':
+            try:
+                open_parens.pop()
+            except IndexError:
+                raise errors.IndexDefinitionParseError(
+                    "Encountered ')' before '(' when parsing:\n%s\n%s^" %
+                    (expression, " " * idx))
+            term = expression[start:idx].strip()
+            if term:
+                tree.append(term)
+            idx += 1
+            start = idx
+            return idx, start, tree
+        elif char == ',':
+            term = expression[start:idx].strip()
+            if term:
+                tree.append(term)
+            idx += 1
+            start = idx
+        else:
+            idx += 1
+    if start < len(expression):
+        tree.append(expression[start:])
+    return idx, start, tree
+
+
+def make_tree(expression):
+    open_parens = []
+    tree = make_subtree(expression, 0, 0, open_parens)[2]
+    if open_parens:
+        raise errors.IndexDefinitionParseError(
+            "%d missing ')'s when parsing '%s'." %
+            (len(open_parens), expression))
+    return tree
+
+
 class Getter(object):
     """Get values from a document based on a specification."""
 
+    arity = 1
+    args = ['expr']
+
     def get(self, raw_doc):
         """Get a value from the document.
 
@@ -93,12 +150,11 @@
     """A transformation on a value from another Getter."""
 
     name = None
-    """The name that the transform has in a query string."""
 
     def __init__(self, inner):
         """Create a transformation.
 
-        :param inner: the Getter to transform the value for.
+        :param inner: the argument(s) to the transformation.
         """
         self.inner = inner
 
@@ -148,6 +204,8 @@
     """
 
     name = 'number'
+    arity = 2
+    args = ['expr', int]
 
     def __init__(self, inner, number):
         super(Number, self).__init__(inner)
@@ -230,44 +288,46 @@
         return partial, ''
 
     def parse(self, field):
-        inner = self._inner_parse(field)
+        tree = make_tree(field)
+        inner = self._inner_parse(tree)
         return inner
 
-    def _inner_parse(self, field):
-        word, field = self._take_word(field)
-        if field.startswith("("):
+    def _inner_parse(self, tree):
+        if len(tree) > 1:
             # We have an operation
-            if not field.endswith(")"):
-                raise errors.IndexDefinitionParseError(
-                    "Invalid transformation function: %s" % field)
-            op = self._transformations.get(word, None)
+            op_name = tree[0]
+            args = tree[1]
+            op = self._transformations.get(op_name, None)
             if op is None:
                 raise errors.IndexDefinitionParseError(
-                    "Unknown operation: %s" % word)
-            if ',' in field:
-                # XXX: The arguments should probably be cast to whatever types
-                # they represent, but short of evaling them, I don't see an
-                # easy way to do that without adding a lot of complexity.
-                # Since there is only one operation with an extra argument, I'm
-                # punting on this until we grow some more.
-                args = [a.strip() for a in field[1:-1].split(',')]
-                extracted = args[0]
-            else:
-                args = []
-                extracted = field[1:-1]
-            inner = self._inner_parse(extracted)
-            return op(inner, *args[1:])
+                    "Unknown operation: %s" % op_name)
+            if op.arity >= 0 and len(args) != op.arity:
+                raise errors.IndexDefinitionParseError(
+                    "Invalid number of arguments for transformation function:"
+                    " %s, %r" % (op_name, args))
+            parsed = []
+            for i, arg in enumerate(args):
+                arg_type = op.args[i % len(op.args)]
+                if arg_type == 'expr':
+                    inner = self._inner_parse([arg])
+                else:
+                    try:
+                        inner = arg_type(arg)
+                    except ValueError, e:
+                        raise errors.IndexDefinitionParseError(
+                            "Invalid value %r for argument type %r (%r)." %
+                            (arg, arg_type, e))
+                parsed.append(inner)
+            return op(*parsed)
         else:
-            if len(field) != 0:
-                raise errors.IndexDefinitionParseError(
-                    "Unhandled characters: %s" % (field,))
-            if len(word) == 0:
-                raise errors.IndexDefinitionParseError(
-                    "Missing field specifier")
-            if word.endswith("."):
-                raise errors.IndexDefinitionParseError(
-                    "Invalid field specifier: %s" % word)
-            return ExtractField(word)
+            if len(tree) == 0:
+                raise errors.IndexDefinitionParseError(
+                    "Expected fieldname or expression.")
+            fieldname = tree[0]
+            if fieldname.endswith("."):
+                raise errors.IndexDefinitionParseError(
+                    "Invalid field specifier: %s" % fieldname)
+            return ExtractField(fieldname)
 
     def parse_all(self, fields):
         return [self.parse(field) for field in fields]

=== modified file 'u1db/tests/test_backends.py'
--- u1db/tests/test_backends.py	2012-07-13 22:09:11 +0000
+++ u1db/tests/test_backends.py	2012-07-17 20:51:27 +0000
@@ -1076,6 +1076,34 @@
         self.assertEqual(
             [doc], self.db.get_from_index('test-idx', 'value', 'value2'))
 
+    def test_get_from_index_multi_list(self):
+        doc = self.db.create_doc(
+            '{"key": "value", "key2": ["value2-1", "value2-2", "value2-3"]}')
+        self.db.create_index('test-idx', 'key', 'key2')
+        self.assertEqual(
+            [doc], self.db.get_from_index('test-idx', 'value', 'value2-1'))
+        self.assertEqual(
+            [doc], self.db.get_from_index('test-idx', 'value', 'value2-2'))
+        self.assertEqual(
+            [doc], self.db.get_from_index('test-idx', 'value', 'value2-3'))
+        self.assertEqual(
+            [('value', 'value2-1'), ('value', 'value2-2'),
+             ('value', 'value2-3')],
+            sorted(self.db.get_index_keys('test-idx')))
+
+    def test_get_index_keys_multi_list_list(self):
+        self.db.create_doc(
+            '{"key": "value1-1 value1-2 value1-3", '
+            '"key2": ["value2-1", "value2-2", "value2-3"]}')
+        self.db.create_index('test-idx', 'split_words(key)', 'key2')
+        self.assertEqual(
+            [(u'value1-1', u'value2-1'), (u'value1-1', u'value2-2'),
+             (u'value1-1', u'value2-3'), (u'value1-2', u'value2-1'),
+             (u'value1-2', u'value2-2'), (u'value1-2', u'value2-3'),
+             (u'value1-3', u'value2-1'), (u'value1-3', u'value2-2'),
+             (u'value1-3', u'value2-3')],
+            sorted(self.db.get_index_keys('test-idx')))
+
     def test_get_from_index_multi_ordered(self):
         doc1 = self.db.create_doc('{"key": "value3", "key2": "value4"}')
         doc2 = self.db.create_doc('{"key": "value2", "key2": "value3"}')

=== modified file 'u1db/tests/test_query_parser.py'
--- u1db/tests/test_query_parser.py	2012-05-15 16:23:19 +0000
+++ u1db/tests/test_query_parser.py	2012-07-17 20:51:27 +0000
@@ -24,6 +24,44 @@
 trivial_raw_doc = {}
 
 
+class TestMakeTree(tests.TestCase):
+
+    def assertParseError(self, definition):
+        self.assertRaises(
+            errors.IndexDefinitionParseError, query_parser.make_tree,
+            definition)
+
+    def test_single_field(self):
+        self.assertEqual(['f'], query_parser.make_tree('f'))
+
+    def test_single_mapping(self):
+        query_parser.make_tree('mapping(field1)')
+        self.assertEqual(
+            ['mapping', ['field1']], query_parser.make_tree('mapping(field1)'))
+
+    def test_single_mapping_multiple_fields(self):
+        self.assertEqual(
+            ['mapping', ['field1', 'field2', 'field3']],
+            query_parser.make_tree('mapping(field1, field2, field3)'))
+
+    def test_nested_mapping(self):
+        self.assertEqual(
+            ['mapping1', ['mapping2', ['field1', 'field2'],
+                          'mapping3', ['field3', 'field4']]],
+            query_parser.make_tree(
+                'mapping1(mapping2(field1, field2), '
+                'mapping3(field3, field4))'))
+
+    def test_parse_missing_close_paren(self):
+        self.assertParseError("lower(a")
+
+    def test_parse_trailing_chars(self):
+        self.assertParseError("lower(ab))")
+
+    def test_parse_empty_op(self):
+        self.assertParseError("(ab)")
+
+
 class TestStaticGetter(tests.TestCase):
 
     def test_returns_string(self):
@@ -322,6 +360,9 @@
     def test_parse_unknown_op(self):
         self.assertParseError("no_such_operation(field)")
 
+    def test_parse_wrong_arg_type(self):
+        self.assertParseError("number(field, fnord)")
+
     def test_parse_transformation(self):
         getter = self.parse("lower(a)")
         self.assertIsInstance(getter, query_parser.Lower)