Merge lp:~thisfred/u1db/number-mapping into lp:u1db

[Eric Casteleijn]

This adds indexing of numeric values as zero padded strings, with the number(field, no_of_zeroes) mapping.

This adds indexing of numeric values as zero padded strings, with the number(field, no_of_zeroes) mapping.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 4/27/2012 2:32 PM, Eric Casteleijn wrote:
> Eric Casteleijn has proposed merging
> lp:~thisfred/u1db/number-mapping into lp:u1db.
>
> Requested reviews: John A Meinel (jameinel) Related bugs: Bug
> #987412 in U1DB: "support indexes on non-string types"
> https://bugs.launchpad.net/u1db/+bug/987412
>
> For more details, see:
> https://code.launchpad.net/~thisfred/u1db/number-mapping/+merge/103862
>
> This adds indexing of numeric values as zero padded strings, with
> the number(field, no_of_zeroes) mapping.

I think this hints that operations should really be more of a
class-like structure. Rather than a function that you pass varargs,
you really want a bit of context along with your function.

I also don't fully understand why number has to be passed in as a
string for every application of 'op_number'. As that means for 50k
documents you call atoi on that number for each doc. It isn't
terrible, but it is surprising.

At one point, I thought you were looking to change the code so we
didn't have to parse the description every time. Did you get anywhere
with that?

+ value = item->data;
+ for (p = value; *p; p++) {
+ if (isdigit(*p) == 0) {
+ continue;
+ }
+ }
+ value_size = strlen(value);
+ if (zeroes > value_size)
+ value_size = zeroes;
+ value_size++;
+ new_value = (char *)calloc(value_size, 1);
+ if (new_value == NULL)
+ {
+ status = U1DB_NOMEM;
+ goto finish;
+ }
+ count = snprintf(new_value, value_size, "%0*d", zeroes,
atoi(value));

I thought originally you had the wrong buffer size, but I missed the
if(zeros > value_size) line. Would it be clearer as:

value_size = max(strlen(value), zeros) + 1;

We haven't clearly specified what should happen if you use number(x,
5) but the number in a document is >=100,000. And I don't see a test
case for a number too big for the width.

...

 trivial_raw_doc = {}
+PADDING = 5

I don't really prefer global constants like this. I would probably do
it as:

+ def assertNumber(self, expected, value, padding=5):

That will also let you easily test a number that is way to big to fit,
and a padding size that is way to big to put all the digits in.

+ for (p = value; *p; p++) {
+ if (isdigit(*p) == 0) {
+ continue;
+ }
+ }

I don't see the point of that loop. You don't do anything with 'p'
after the loop.

Maybe that is the 'p' that you check earlier? And you forgot and left
this loop in?

...

+static char *
+itoa(int integer_value)
+{

Because the maximum size of an integer is known in advance
(len(str(2**64)) == 20), I think we would be better off avoiding
counting the size of the integer and doing a calloc + free, and just
having a buffer of size 21, that gets allocated on the stack and
passed into something like 'snprintf(buf, 21, "%d", val)'.

So that would make this code:

+ } else if (json_object_is_type(val, json_type_int)) {
+ integer_value = json_object_get_int(val);
+ string_value = itoa(integer_value);
+ ...

[Eric Casteleijn (thisfred)]

I addressed all of your comments, so please have another look, to see if you agree with the direction I'm taking if nothing else.

Known issues:

- there is some memory leaking, but make valgrind-leaks does not fail any tests... (I will chase this down tomorrow)
- the parsing of the field_mappings still happens once for every document. This is now no longer necessary, but lifting the transformations out of evaluate_index_and_insert_into_db is likely to make the branch quite a bit bigger. (I tried but it means changing the callback signature, and that affected quite a bit of the code.) Note that the python implementation *also* parses the expression once per document, so we may want to fix that as well.

[lp:~thisfred/u1db/number-mapping]

267. By Eric Casteleijn on 2012-05-01

added pyxdg

268. By Eric Casteleijn on 2012-05-01

merged trunk and fixed conflict

269. By Eric Casteleijn on 2012-05-01

added test

270. By Eric Casteleijn on 2012-05-01

fixed memory leak

[Eric Casteleijn (thisfred)]

Memory issue found and fixed.

[John A Meinel (jameinel)]

I'm thinking the max() code is defined elsewhere as well, we probably want it in a more common header, like maybe u1db_internal.h

111 + if (tr->args->head != NULL)
112 + {
113 + status = ((args_operation)tr->op)(result, tmp_values, tr->args);
114 + } else {
115 + status = ((operation)tr->op)(result, tmp_values);
116 + }

I think I'd rather just have all operations take a void * that could be the args list, rather than having this code special case. Just some operations would have a NULL args list.

I feel like I'd rather have the args as a void* in the transformation code, and have it be a struct that is defined by the type, rather than a generic linked-list of string values. For now this is fine, but it feels like a better place to end up at. (Maybe you have more of that in the next steps)

...
char string_value[21];

Thinking about this, we should pull the 21 out to be a defined constant with a comment about "max length of a decimal integer".

...

466 + status = init_transformation(&inner);
467 + if (status != U1DB_OK)
468 + goto finish;
469 + status = parse(new_ptr, inner);
470 + if (status != U1DB_OK)
471 + {
472 + destroy_transformation(inner);
473 + goto finish;
474 + }
475 + result->next = inner;

I only see a code path that calls "destroy_transformation" if the status is not U1DB_OK. Maybe I'm missing something because the transformation is a linked list?

...

91 +static void
92 +destroy_transformation(transformation *tr)
93 +{
94 + if (tr->next != NULL)

You don't have a check here:

if (tr == NULL) return;

Because if you go to parse a sequence, and you get an error with the first item, you'll never set a result, I think.
Actually, it looks like that only triggers if you fail to init the transformation in the first place, but it still seems to be a valid concern.

Otherwise, I'm happy enough.

[Eric Casteleijn (thisfred)]

> I'm thinking the max() code is defined elsewhere as well, we probably want it
> in a more common header, like maybe u1db_internal.h

I'll have a search. Mine might not be safe to use in all contexts.

> 111 + if (tr->args->head != NULL)
> 112 + {
> 113 + status = ((args_operation)tr->op)(result, tmp_values, tr->args);
> 114 + } else {
> 115 + status = ((operation)tr->op)(result, tmp_values);
> 116 + }
>
>
> I think I'd rather just have all operations take a void * that could be the
> args list, rather than having this code special case. Just some operations
> would have a NULL args list.
>
> I feel like I'd rather have the args as a void* in the transformation code,
> and have it be a struct that is defined by the type, rather than a generic
> linked-list of string values. For now this is fine, but it feels like a better
> place to end up at. (Maybe you have more of that in the next steps)

The arguments are parsed out of the index expression, but unless we start annotating the operations with how many and what types of arguments they expect, or hardcode lots of conditional logic about the different operators in the parser, I don't see how we can do much more than split on commas and let the operations deal with that list of strings. As we accumulate more operations, having this kind of type annotation may become worth the effort, but since we now have exactly one operation with a single extra argument, I don't think it's time to make that call yet. I will change the extra argument to a void which we can pass a NULL to in the usual case, because that is a definite improvement.

> ...
> char string_value[21];
>
> Thinking about this, we should pull the 21 out to be a defined constant with a
> comment about "max length of a decimal integer".

will do

> ...
>
> 466 + status = init_transformation(&inner);
> 467 + if (status != U1DB_OK)
> 468 + goto finish;
> 469 + status = parse(new_ptr, inner);
> 470 + if (status != U1DB_OK)
> 471 + {
> 472 + destroy_transformation(inner);
> 473 + goto finish;
> 474 + }
> 475 + result->next = inner;
>
> I only see a code path that calls "destroy_transformation" if the status is
> not U1DB_OK. Maybe I'm missing something because the transformation is a
> linked list?

I'll add a comment, but yeah: if the creation of the inner transformation succeeds, it will be cleaned up recursively when the outermost one is destroyed.

> ...
>
> 91 +static void
> 92 +destroy_transformation(transformation *tr)
> 93 +{
> 94 + if (tr->next != NULL)
>
> You don't have a check here:
>
> if (tr == NULL) return;
>
> Because if you go to parse a sequence, and you get an error with the first
> item, you'll never set a result, I think.
> Actually, it looks like that only triggers if you fail to init the
> transformation in the first place, but it still seems to be a valid concern.

Will fix.

[lp:~thisfred/u1db/number-mapping]

271. By Eric Casteleijn on 2012-05-02

simplification of operations

272. By Eric Casteleijn on 2012-05-02

added comment and fixed destroy transformation

[Eric Casteleijn (thisfred)]

> ... I will change the extra argument
> to a void which we can pass a NULL to in the usual case, because that is a
> definite improvement.

Actually, if it's always a string_list or null, we might as well keep the type. I did collapse the three different operation types back into one though, since that made no sense.

[John A Meinel (jameinel)]

review: Approve

=== modified file 'include/u1db/u1db.h'
--- include/u1db/u1db.h	2012-04-30 11:19:18 +0000
+++ include/u1db/u1db.h	2012-05-02 14:10:25 +0000
@@ -58,6 +58,11 @@
 #define U1DB_INVALID_VALUE_FOR_INDEX -9
 #define U1DB_INVALID_HTTP_RESPONSE -10
 #define U1DB_BROKEN_SYNC_STREAM -11
+#define U1DB_INVALID_TRANSFORMATION_FUNCTION -12
+#define U1DB_UNKNOWN_OPERATION -13
+#define U1DB_UNHANDLED_CHARACTERS -14
+#define U1DB_MISSING_FIELD_SPECIFIER -15
+#define U1DB_INVALID_FIELD_SPECIFIER -16
 #define U1DB_INTERNAL_ERROR -999
 
 // Used by put_doc_if_newer

=== modified file 'setup.py'
--- setup.py	2012-04-17 14:51:19 +0000
+++ setup.py	2012-05-02 14:10:25 +0000
@@ -35,7 +35,7 @@
         "package_data": {'': ["*.sql"]},
         "scripts": ['u1db-client', 'u1db-serve'],
         "ext_modules": ext,
-        "install_requires": ["paste", "simplejson", "routes"],
+        "install_requires": ["paste", "simplejson", "routes", "pyxdg"],
         # informational
         "tests_require": ["testtools", "testscenarios", "Cython"],
         "classifiers": [

=== modified file 'src/u1db_query.c'
--- src/u1db_query.c	2012-04-30 11:17:22 +0000
+++ src/u1db_query.c	2012-05-02 14:10:25 +0000
@@ -23,8 +23,11 @@
 #include <ctype.h>
 #include <json/json.h>
 
-#define OPERATIONS 2
-static const char *OPERATORS[OPERATIONS] = {"lower", "split_words"};
+#define OPERATIONS 3
+#ifndef max
+    #define max(a, b) (((a) > (b)) ? (a) : (b))
+#endif
+static const char *OPERATORS[OPERATIONS] = {"lower", "number", "split_words"};
 
 typedef struct string_list_item_
 {
@@ -38,11 +41,24 @@
     string_list_item *tail;
 } string_list;
 
-typedef int(*operation)(string_list *, const string_list *);
-static int op_lower(string_list *result, const string_list *value);
-static int op_split_words(string_list *result, const string_list *value);
-
-static operation operations[OPERATIONS] = {op_lower, op_split_words};
+typedef struct transformation_
+{
+    void *op;
+    struct transformation_ *next;
+    string_list *args;
+} transformation;
+
+typedef int(*operation)(string_list *, const string_list *,
+                        const string_list *);
+static int op_lower(string_list *result, const string_list *value,
+                    const string_list *args);
+static int op_number(string_list *result, const string_list *value,
+                     const string_list *args);
+static int op_split_words(string_list *result, const string_list *value,
+                          const string_list *args);
+
+static operation operations[OPERATIONS] = {
+    op_lower, op_number, op_split_words};
 
 
 static int
@@ -97,6 +113,121 @@
 }
 
 static int
+init_transformation(transformation **tr)
+{
+    int status = U1DB_OK;
+    if ((*tr = (transformation *)malloc(sizeof(transformation))) == NULL)
+        return U1DB_NOMEM;
+    (*tr)->op = NULL;
+    (*tr)->next = NULL;
+    status = init_list(&((*tr)->args));
+    if (status != U1DB_OK)
+        return status;
+    return status;
+}
+
+static void
+destroy_transformation(transformation *tr)
+{
+    if (tr == NULL)
+        return;
+    if (tr->next != NULL)
+        destroy_transformation(tr->next);
+    destroy_list(tr->args);
+    free(tr);
+}
+
+static int
+extract_field_values(string_list *values, json_object *obj,
+                     const string_list *field_path)
+{
+    string_list_item *item = NULL;
+    char string_value[21];
+    struct array_list *list_val = NULL;
+    json_object *val = NULL;
+    int i, integer_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)) {
+        if ((status = append(values, json_object_get_string(val))) != U1DB_OK)
+            goto finish;
+    } else if (json_object_is_type(val, json_type_int)) {
+        integer_value = json_object_get_int(val);
+        snprintf(string_value, 21, "%d", integer_value);
+        if (status != U1DB_OK)
+            goto finish;
+        if ((status = append(values, string_value)) != U1DB_OK)
+            goto finish;
+    } else if (json_object_is_type(val, json_type_array)) {
+        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;
+}
+
+
+static int
+apply_transformation(transformation *tr, json_object *obj, string_list *result)
+{
+    int status = U1DB_OK;
+    string_list *tmp_values = NULL;
+    if (tr->next != NULL)
+    {
+        init_list(&tmp_values);
+        status = apply_transformation(tr->next, obj, tmp_values);
+        if (status != U1DB_OK)
+            goto finish;
+        if (tr->args->head != NULL)
+        {
+            status = ((operation)tr->op)(result, tmp_values, tr->args);
+        } else {
+            status = ((operation)tr->op)(result, tmp_values, NULL);
+        }
+    } else {
+        status = extract_field_values(result, obj, tr->args);
+        goto finish;
+    }
+finish:
+    destroy_list(tmp_values);
+    return status;
+}
+
+static int
+split(string_list *result, char *string, char splitter)
+{
+    int status = U1DB_OK;
+    char *result_ptr, *split_point;
+    result_ptr = string;
+    while (result_ptr != NULL) {
+        split_point = strchr(result_ptr, splitter);
+        if (split_point != NULL) {
+            *split_point = '\0';
+            split_point++;
+        }
+        status = append(result, result_ptr);
+        if (status != U1DB_OK)
+            return status;
+        result_ptr = split_point;
+    }
+    return status;
+}
+
+static int
 list_index(string_list *list, char *data)
 {
     int i = 0;
@@ -113,20 +244,22 @@
 }
 
 static int
-list_copy(string_list *copy, const string_list *original)
+is_word_char(char c)
 {
-    string_list_item *item = NULL;
-    int status = U1DB_OK;
-    for (item = original->head; item != NULL; item = item->next)
-        if ((status = append(copy, item->data)) != U1DB_OK)
-        {
-            return status;
-        }
-    return status;
+    if (isalnum(c))
+    {
+        return 0;
+    }
+    if (c == '.')
+        return 0;
+    if (c == '_')
+        return 0;
+    return -1;
 }
 
 static int
-op_lower(string_list *result, const string_list *values)
+op_lower(string_list *result, const string_list *values,
+         const string_list *args)
 {
     string_list_item *item = NULL;
     char *new_value, *value = NULL;
@@ -159,7 +292,63 @@
 }
 
 static int
-op_split_words(string_list *result, const string_list *values)
+op_number(string_list *result, const string_list *values,
+          const string_list *args)
+{
+    string_list_item *item = NULL;
+    char *p, *new_value, *value, *number = NULL;
+    int count, zeroes, value_size, isnumber;
+    int status = U1DB_OK;
+
+    number = args->head->data;
+    for (p = number; *p; p++) {
+        if (isdigit(*p) == 0) {
+            status = U1DB_INVALID_VALUE_FOR_INDEX;
+            goto finish;
+        }
+    }
+    zeroes = atoi(number);
+
+    for (item = values->head; item != NULL; item = item->next)
+    {
+        value = item->data;
+        isnumber = 1;
+        for (p = value; *p; p++) {
+            if (isdigit(*p) == 0) {
+                isnumber = 0;
+                break;
+            }
+        }
+        if (isnumber == 0) {
+            continue;
+        }
+        value_size = max(strlen(value), zeroes) + 1;
+        new_value = (char *)calloc(value_size, 1);
+        if (new_value == NULL)
+        {
+            status = U1DB_NOMEM;
+            goto finish;
+        }
+        count = snprintf(new_value, value_size, "%0*d", zeroes, atoi(value));
+        if (count != (value_size - 1)) {
+            // Most likely encoding issues.
+            status = U1DB_INVALID_PARAMETER;
+            goto finish;
+        }
+        if ((status = append(result, new_value)) != U1DB_OK)
+        {
+            free(new_value);
+            goto finish;
+        }
+        free(new_value);
+    }
+finish:
+    return status;
+}
+
+static int
+op_split_words(string_list *result, const string_list *values,
+               const string_list *args)
 {
     string_list_item *item = NULL;
     char *intermediate, *intermediate_ptr = NULL;
@@ -560,128 +749,124 @@
 }
 
 static int
-extract_field_values(const char *expression, string_list *values,
-        json_object *obj)
-{
-    char *lparen, *rparen, *sub = NULL;
-    char *result, *result_ptr, *dot_chr = NULL;
-    struct array_list *list_val = NULL;
-    json_object *val = NULL;
-    int path_size, i;
-    int status = U1DB_OK;
-    lparen = strchr(expression, '(');
-    if (lparen == NULL)
-    {
-        result = strdup(expression);
-        result_ptr = result;
-        val = obj;
-        while (result_ptr != NULL && val != NULL) {
-            dot_chr = strchr(result_ptr, '.');
-            if (dot_chr != NULL) {
-                *dot_chr = '\0';
-                dot_chr++;
-            }
-            // TODO: json_object uses ref-counting. Do we need to be
-            //       json_object_put to the previous val so it gets cleaned up
-            //       appropriately?
-            val = json_object_object_get(val, result_ptr);
-            result_ptr = dot_chr;
-        }
-        if (result != NULL)
-        {
-            free(result);
-        }
-        if (val == NULL)
-        {
-            return U1DB_OK;
-        }
-        if (json_object_is_type(val, json_type_string)) {
-            if ((status = append(values, json_object_get_string(val))) != U1DB_OK)
-            {
-                return status;
-            }
-        } else if (json_object_is_type(val, json_type_array)) {
-            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)
-                {
-                    return status;
-                }
-            }
-        } else {
-        }
-        // TODO: this segfaults unreliably:
-        // json_object_put(val);
-        return U1DB_OK;
-    }
-    rparen = strrchr(expression, ')');
-    if (rparen == NULL)
-    {
-        return U1DB_INVALID_VALUE_FOR_INDEX;
-    }
-    path_size = ((rparen - 1) - (lparen + 1)) + 1;
-    sub = (char *)calloc(path_size + 1, 1);
-    if (sub != NULL)
-    {
-        strncpy(sub, lparen + 1, path_size);
-        sub[path_size] = '\0';
-        status = extract_field_values(sub, values, obj);
-        free(sub);
-    }
-    return status;
-}
-
-static int
-apply_operations(const char *expression, string_list *result,
-        const string_list *values)
-{
-    operation op = NULL;
-    char *lparen, *op_name = NULL;
-    int i, op_size;
-    int status = U1DB_OK;
-    string_list *tmp_values = NULL;
-    lparen = strchr(expression, '(');
-    if (lparen == NULL)
-    {
-        if ((status = list_copy(result, values)) != U1DB_OK)
-            return status;
-        return U1DB_OK;
-    }
-    op_size = ((lparen - 1) - expression) + 1;
-    op_name = (char *)calloc(op_size + 1, 1);
-    if (op_name != NULL)
-    {
-        strncpy(op_name, expression, op_size);
-        op_name[op_size] = '\0';
+parse(const char *field, transformation *result)
+{
+    transformation *inner = NULL;
+    char *new_field, *new_ptr, *argptr, *argend, *word, *first_comma = NULL;
+    int status = U1DB_OK;
+    int i, size;
+    char *field_copy, *end = NULL;
+    field_copy = strdup(field);
+    end = field_copy;
+    while (is_word_char(*end) == 0)
+    {
+        end++;
+    }
+    if (*end == '\0')
+    {
+        word = strdup(field_copy);
+        if (word == NULL)
+        {
+            status = U1DB_NOMEM;
+            goto finish;
+        }
+    }
+    else {
+        // TODO: unicode fieldnames ?
+        size = (end - field_copy);
+        word = (char *)calloc(size + 1, 1);
+        strncpy(word, field_copy, size);
+    }
+    new_field = strdup(end);
+    new_ptr = new_field;
+    if (status != U1DB_OK)
+        goto finish;
+    if (*new_ptr == '(')
+    {
+        if (new_field[strlen(new_field) - 1] != ')')
+        {
+            status = U1DB_INVALID_TRANSFORMATION_FUNCTION;
+            goto finish;
+        }
+        // step into parens
+        new_ptr++;
+        new_field[strlen(new_field) - 1] = '\0';
         for (i = 0; i < OPERATIONS; i++)
         {
-            if (strcmp(OPERATORS[i], op_name) == 0)
+            if (strcmp(OPERATORS[i], word) == 0)
             {
-                op = operations[i];
+                result->op = operations[i];
                 break;
             }
         }
-        if (op == NULL)
-        {
-            status = U1DB_INVALID_VALUE_FOR_INDEX;
-            goto finish;
-        }
-        if ((status = init_list(&tmp_values)) != U1DB_OK)
-            goto finish;
-        if ((status = apply_operations(lparen + 1, tmp_values, values)) !=
-                U1DB_OK)
-            goto finish;
-        op(result, tmp_values);
+        if (result == NULL)
+        {
+            status = U1DB_UNKNOWN_OPERATION;
+            goto finish;
+        }
+        first_comma = strchr(new_field, ',');
+        if (first_comma != NULL)
+        {
+            argptr = first_comma + 1;
+            argend = argptr;
+            *first_comma = '\0';
+            while (*argend != '\0')
+            {
+                if (*argend == ',')
+                {
+                    *argend = '\0';
+                    while (*argptr == ' ')
+                        argptr++;
+                    status = append(result->args, argptr);
+                    if (status != U1DB_OK)
+                        goto finish;
+                    argptr = argend + 1;
+                }
+                argend++;
+            }
+            if ((argend - argptr) > 0)
+            {
+                while (*argptr == ' ')
+                    argptr++;
+                status = append(result->args, argptr);
+                if (status != U1DB_OK)
+                    goto finish;
+            }
+        }
+        status = init_transformation(&inner);
+        if (status != U1DB_OK)
+            goto finish;
+        status = parse(new_ptr, inner);
+        if (status != U1DB_OK)
+        {
+            // free the memory if the parsing fails. Otherwise, inner will be
+            // cleaned up when the outer transformation (result) is destroyed.
+            destroy_transformation(inner);
+            goto finish;
+        }
+        result->next = inner;
+    } else {
+        if (*new_ptr != '\0')
+        {
+            status = U1DB_UNHANDLED_CHARACTERS;
+            goto finish;
+        }
+        if (strlen(word) == 0)
+        {
+            status = U1DB_MISSING_FIELD_SPECIFIER;
+            goto finish;
+        }
+        if (word[strlen(word) - 1] == '.')
+        {
+            status = U1DB_INVALID_FIELD_SPECIFIER;
+            goto finish;
+        }
+        status = split(result->args, word, '.');
     }
 finish:
-    destroy_list(tmp_values);
-    if (op_name != NULL)
-    {
-        free(op_name);
-    }
+    free(word);
+    free(field_copy);
+    free(new_field);
     return status;
 }
 
@@ -689,7 +874,8 @@
 evaluate_index_and_insert_into_db(void *context, const char *expression)
 {
     struct evaluate_index_context *ctx;
-    string_list *tmp_values, *values = NULL;
+    transformation *tr = NULL;
+    string_list *values = NULL;
     string_list_item *item = NULL;
     int status = U1DB_OK;
 
@@ -697,19 +883,15 @@
     if (ctx->obj == NULL || !json_object_is_type(ctx->obj, json_type_object)) {
         return U1DB_INVALID_JSON;
     }
-    if ((status = init_list(&tmp_values)) != U1DB_OK)
-        goto finish;
-    if ((status = extract_field_values(expression, tmp_values, ctx->obj)) !=
-            U1DB_OK)
-    {
-        goto finish;
-    }
+    status = init_transformation(&tr);
+    if (status != U1DB_OK)
+        goto finish;
+    status = parse(expression, tr);
+    if (status != U1DB_OK)
+        goto finish;
     if ((status = init_list(&values)) != U1DB_OK)
         goto finish;
-    if ((status = apply_operations(expression, values, tmp_values)) != U1DB_OK)
-    {
-        goto finish;
-    }
+    status = apply_transformation(tr, ctx->obj, values);
     for (item = values->head; item != NULL; item = item->next)
     {
         if ((status = add_to_document_fields(ctx->db, ctx->doc_id, expression,
@@ -717,8 +899,8 @@
             goto finish;
     }
 finish:
-    destroy_list(tmp_values);
     destroy_list(values);
+    destroy_transformation(tr);
     return status;
 }
 

=== modified file 'u1db/query_parser.py'
--- u1db/query_parser.py	2011-12-21 19:33:35 +0000
+++ u1db/query_parser.py	2012-05-02 14:10:25 +0000
@@ -141,6 +141,30 @@
         return [val.lower() for val in values if self._can_transform(val)]
 
 
+class Number(Transformation):
+    """Convert an integer to a zero padded string.
+
+    This transformation will return None for non-integer inputs. However, it
+    will transform any integers in a list, dropping any elements that are not
+    integers.
+    """
+
+    name = 'number'
+
+    def __init__(self, inner, number):
+        super(Number, self).__init__(inner)
+        self.padding = "%%0%sd" % number
+
+    def _can_transform(self, val):
+        return not isinstance(val, (str, bool, float, list, dict))
+
+    def transform(self, values):
+        """Transform any integers in values into zero padded strings."""
+        if not values:
+            return []
+        return [self.padding % (v,) for v in values if self._can_transform(v)]
+
+
 class SplitWords(Transformation):
     """Split a string on whitespace.
 
@@ -187,7 +211,6 @@
     _word_chars = string.lowercase + string.uppercase + "._" + string.digits
 
     def _take_word(self, partial):
-        word = ''
         for idx, char in enumerate(partial):
             if char not in self._word_chars:
                 return partial[:idx], partial[idx:]
@@ -208,8 +231,19 @@
             if op is None:
                 raise errors.IndexDefinitionParseError(
                     "Unknown operation: %s" % word)
-            inner = self._inner_parse(field[1:-1])
-            return op(inner)
+            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:])
         else:
             if len(field) != 0:
                 raise errors.IndexDefinitionParseError(
@@ -235,4 +269,5 @@
 
 Parser.register_transormation(SplitWords)
 Parser.register_transormation(Lower)
+Parser.register_transormation(Number)
 Parser.register_transormation(IsNull)

=== modified file 'u1db/tests/test_backends.py'
--- u1db/tests/test_backends.py	2012-04-30 11:19:18 +0000
+++ u1db/tests/test_backends.py	2012-05-02 14:10:25 +0000
@@ -840,7 +840,7 @@
     def test_index_lower_doesnt_match_different_case(self):
         self.db.create_index("index", ["lower(name)"])
         content = '{"name": "Foo"}'
-        doc = self.db.create_doc(content)
+        self.db.create_doc(content)
         rows = self.db.get_from_index("index", [("Foo", )])
         self.assertEqual([], rows)
 
@@ -848,7 +848,7 @@
         self.db.create_index("index", ["lower(name)"])
         self.db.create_index("other_index", ["name"])
         content = '{"name": "Foo"}'
-        doc = self.db.create_doc(content)
+        self.db.create_doc(content)
         rows = self.db.get_from_index("index", [("Foo", )])
         self.assertEqual(0, len(rows))
 
@@ -894,6 +894,20 @@
         rows = self.db.get_from_index("index", [("bar", )])
         self.assertEqual([doc], rows)
 
+    def test_get_from_index_with_number(self):
+        self.db.create_index("index", ["number(foo, 5)"])
+        content = '{"foo": 12}'
+        doc = self.db.create_doc(content)
+        rows = self.db.get_from_index("index", [("00012", )])
+        self.assertEqual([doc], rows)
+
+    def test_get_from_index_with_number_bigger_than_padding(self):
+        self.db.create_index("index", ["number(foo, 5)"])
+        content = '{"foo": 123456}'
+        doc = self.db.create_doc(content)
+        rows = self.db.get_from_index("index", [("123456", )])
+        self.assertEqual([doc], rows)
+
     def test_get_index_keys_from_index(self):
         self.db.create_index('test-idx', ['key'])
         content1 = '{"key": "value1"}'

=== modified file 'u1db/tests/test_query_parser.py'
--- u1db/tests/test_query_parser.py	2011-12-21 19:33:35 +0000
+++ u1db/tests/test_query_parser.py	2012-05-02 14:10:25 +0000
@@ -184,6 +184,63 @@
                               ['foo baz', {'baa': 'xam'}, 'bar sux'])
 
 
+class TestNumber(tests.TestCase):
+
+    def assertNumber(self, expected, value, padding=5):
+        """Assert number transformation produced expected values."""
+        getter = query_parser.Number(query_parser.StaticGetter(value), padding)
+        self.assertEqual(expected, getter.get(trivial_raw_doc))
+
+    def test_inner_returns_None(self):
+        """None is thrown away."""
+        self.assertNumber([], None)
+
+    def test_inner_returns_int(self):
+        """A single integer is converted to zero padded strings."""
+        self.assertNumber(['00009'], 9)
+
+    def test_inner_returns_list(self):
+        """Integers are converted to zero padded strings."""
+        self.assertNumber(['00009', '00235'], [9, 235])
+
+    def test_inner_returns_string(self):
+        """A string is thrown away."""
+        self.assertNumber([], 'foo bar')
+
+    def test_inner_returns_float(self):
+        """A float is thrown away."""
+        self.assertNumber([], 9.2)
+
+    def test_inner_returns_bool(self):
+        """A boolean is thrown away."""
+        self.assertNumber([], True)
+
+    def test_inner_returns_list_containing_strings(self):
+        """Strings in a list are thrown away."""
+        self.assertNumber(['00009'], ['foo baz', 9, 'bar sux'])
+
+    def test_inner_returns_list_containing_float(self):
+        """Floats in a list are thrown away."""
+        self.assertNumber(
+            ['00083', '00073'], [83, 9.2, 73])
+
+    def test_inner_returns_list_containing_bool(self):
+        """Booleans in a list are thrown away."""
+        self.assertNumber(
+            ['00083', '00073'], [83, True, 73])
+
+    def test_inner_returns_list_containing_list(self):
+        """Lists in a list are thrown away."""
+        # TODO: Expand sub-lists?
+        self.assertNumber(
+            ['00012', '03333'], [12, [29], 3333])
+
+    def test_inner_returns_list_containing_dict(self):
+        """Dicts in a list are thrown away."""
+        self.assertNumber(
+            ['00012', '00001'], [12, {54: 89}, 1])
+
+
 class TestIsNull(tests.TestCase):
 
     def assertIsNull(self, value):