[bandicoot] primitive parameters in bandicoot functions

[Julius Chrobak]

hi,

this is a proposal patch to implement primitive parameters for bandicoot functions. This change allows to execute HTTP GET/POST with parameters in the URL and use them for select and extend operators. The grammar change allows to specify multiple primitive parameters as well as one relational parameter for a function.

Feel free to comment on this change and propose any improvements.

Julius


Example 1:

bandicoot function :
---------------
rel Data {
x: int
}

d: Data;

fn Test(i: int): Data
{
return d select(x == i);
}
---------------

curl request example:
---------------
[~] curl http://localhost:12345/Test?i=1
x:int
[~] 
---------------


Example 2:

bandicoot function:
---------------
rel Data {
x: int
}

fn Test(d: Data, i: int): Data
{
return d select(x == i);
}
---------------

curl request example:
---------------
[~] echo -e "x:int\n1\n2" | curl --data-binary @- http://localhost:12345/Test?i=1
x:int
1
[~] 
---------------

[david dempster]

6 emails and counting already this morning Mr spammer 8-)

[Julius Chrobak]

Yes I know and I'm really sorry about it :(

[david dempster]

Haha no problem. Nice to hear from you even if it is spam.

[Michel Martens]

On Wed, Jul 13, 2011 at 3:37 AM, Julius Chrobak <ju...@bandilab.org> wrote:
> hi,
>
> this is a proposal patch to implement primitive parameters for bandicoot
> functions. This change allows to execute HTTP GET/POST with parameters in
> the URL and use them for select and extend operators. The grammar change
> allows to specify multiple primitive parameters as well as one relational
> parameter for a function.
>
> Feel free to comment on this change and propose any improvements.

Another idea would be to overload the operators used in select and
extend to allow the use of rels, given that the rel has only one
attribute and is of the correct type.

Example 3:

bandicoot function:
---------------
rel Item {
id: int,
ts: long,
}

queue: Item;

rel Timestamp {
ts: long
}

fn recent(t: Timestamp): Item
{
return queue select(ts > t);
}
---------------

curl request example:
---------------
[~] echo -e "ts:long\n1312027737" | curl --data-binary @-
http://localhost:12345/recent
id:int,ts:long
[~]
---------------

In the example, it doesn't matter that the name of the attribute is
also 'ts', it only matters that the rel can be coerced into a value of
type long.

The advantage in this example is that the API is unified, and in my
mind it makes sense to use relations instead of primitive types in
both select and extend. (The case for hardcoded values in select and
extend is not something that I have used much in my short experience
with bandicoot).

[Michel Martens]

On Sat, Jul 30, 2011 at 9:17 AM, Michel Martens <mic...@soveran.com> wrote:
>> Feel free to comment on this change and propose any improvements.
>
> Another idea would be to overload the operators used in select and
> extend to allow the use of rels, given that the rel has only one
> attribute and is of the correct type.

Sorry for the noise: I've been playing with the original patch and I
like it a lot, so you can probably diregard my proposal. My idea was
to keep the same API for all parameters, but it may not make sense.

[Ostap Cherkashin]

I would not disregard your proposal straight-away. There are a couple of things which it does not take into account though, like multiple attributes and multiple tuples (e.g. shall it evaluate each tuple comparison from the input using boolean OR or AND), etc.

Relational model defines a theta-join operator which is very similar to what you suggested and it also answers the questions above. E.g. your example could be rewritten as follows (note this is a draft syntax and bandicoot does not support this operator at the moment):

rel Item {
id: int,
ts: long,
}

queue: Item;

rel Timestamp {
ts: long
}

fn recent(t: Timestamp): Item
{
return (queue, t) join(left.ts > right.ts);
}

See here for more info: http://en.wikipedia.org/wiki/Relational_algebra#.CE.B8-join_and_equijoin

- Ostap

[Ostap Cherkashin]

Primitive parameters in bandicoot functions make it easier to implement parameterized select and extend queries and even combine the two in a convenient way, so IMO this functionality is a great extension. I went through the patch and written down some implementation comments:

1.) http related:

1.1.) http 405: this error code stands for "Method Not Allowed" while this patch uses it in the following (extra) cases:
1.1.1.) number of primitive parameters does not match the function declaration
1.1.2.) primitive parameters names do not match the function declaration
1.1.3.) primitive parameters values parsing problems (more on this below)

1.2.) handling of duplicate primitive parameters; e.g. a query string "name=a&name=b&name=c" results in http 400 while this is a correct request. usually implementations return some kind of collection in this case, but its not clear what to do in case of bandicoot; it is better to ignore all but the first parameter rather than return http 400.

1.3.) http errors are not verbose enough (as it is without the patch), and now with the addition of primitive parameters the situation gets even worse; we are supposed to return entity body with the error description (this could also help to resolve 1.1)

1.4.) with the introduction of primitive parameters the following fixme becomes more of a real problem (http.c):
---
/* FIXME: we should read until '\n' or until the buffer is exhausted */
---
e.g. if a client sends a request with the primitive parameters and his/her cookies it is fairly easy to exceed the 8192 byte limit.

1.5.) primitive parameters parsing in bandicoot.c introduces another way of working with values, yet it is very similar to the pack routine (which is supposed to do exactly the same):
--- pack.c
129 int error = 0;
130 if (t == Int) {
131 v_ints[v_int_cnt] = str_int(str_vals[j], &error);
132 v[pos] = val_new_int(&v_ints[v_int_cnt++]);
133 } else if (t == Real) {
134 v_reals[v_real_cnt] = str_real(str_vals[j], &error);
135 v[pos] = val_new_real(&v_reals[v_real_cnt++]);
136 } else if (t == Long) {
137 v_longs[v_long_cnt] = str_long(str_vals[j], &error);
138 v[pos] = val_new_long(&v_longs[v_long_cnt++]);
139 } else if (t == String) {
140 error = str_len(str_vals[j]) > MAX_STRING;
141 v[pos] = val_new_str(str_vals[j]);
142 }
--- bandicoot.c (after applying the patch)
193 char *val = req->args->vals[idx];
194 int error = 0;
195 if (t == Int) {
196 arg->vals[i].v_int = str_int(val, &error);
197 } else if (t == Real)
198 arg->vals[i].v_real = str_real(val, &error);
199 else if (t == Long)
200 arg->vals[i].v_long = str_long(val, &error);
201 else if (t == String) {
202 error = str_len(val) > MAX_STRING;
203 if (!error)
204 str_cpy(arg->vals[i].v_str, val);
205 }
---
in both cases the primitive values are parsed from a string. it makes sense to unify these two to avoid code duplicity.

1.6.) http.c introduces convert() function to decode the query string parameters; this functionality is then spread between http.c and string.c:hex_to_char; it makes more sense to add just one function string.c:str_urldecode (standard name for this functionality)
1.6.1.) also, the + sign in the query string is frequently interpreted as the space character (though rfc1738 does not mention any special handling of it); probably it is alright to leave it as is with %XX only; any further comments on this?

2.) memory related:
2.1.) test/http (args_parse) has invalid reads/writes (on linux 64-bit)
2.2.) test/relation and test/transaction have memory leaks, but it looks like they were introduced with one of the previous patches

3.) misc
3.1.) Http_Args can be inlined into Http_Req => no abuse of the global name space
3.2.) there are some trailing spaces left
3.3.) Func uses very short member names while rel_param and prim_param are not inline with that; the best is to rename the rest of the members to longer names, but using something like rp and pp and comments is also alright.

[julo]

Thank you a lot for the detailed review. It looks like there are a lot
of improvements required. I'd like to sort the points according to
their severity and see how much I can get done for the v4 release.

Here is my suggestion in regards to the priorities (starting with the
most important one) and my comments as well:

1.3) solve the problem with the entity body on a failure response

1.1) return the proper error code including more information about the
error

1.2) I would suggest to return error with additional information
rather than take implicitly the first value. If we agree on how to
handle multiple values for the same parameter name we can implement as
a separate change.

1.4) handle HTTP headers bigger than 8192 bytes

1.6) excluding 1.6.1) In regards to the + sing I'd suggest to leave it
as it is.

3) + 1.5) as a general code clean up point

2) fixing mem leaks in tests

Looking at the priority it looks to me that points 1.3, 1.1, and 1.2
should be done for the v4 version. The rest could be left out and
fixed in the following patches and included in v5 release.

Unless someone completely disagree with me I'll focus on fixing the
first three points.

[Julius Chrobak]

I've created a remove branch called 'params' to track the changes on
this subject.
I've also implemented points 1.1, 1.2, and 1.3.

Bandicoot is now able to return more information in case of HTTP 404
errors. The details
are returned in an HTTP response within the entity body. The format is
our standard CSV
representation of the following relation:

rel {
code: int,
msg: string
}

Example: - calling a non existing function results in the following
HTTP response:
-------------------------
HTTP/1.1 404 Not Found
Content-Type: text/plain
Content-Length: 41

code:int,msg:string
1,unknown function
-------------------------

I've decided to use solely the 404 error code to cover all the
different error situations
related the primitive parameters. I was also thinking about using 400
error but
according to the spec the 400 error is for requests with a malformed
syntax.

I'm still working on the outstanding points with the focus on 1.4. As
soon as this is
implemented I'll push the changes to the branch and let you know.

[Julius Chrobak]

I've finished additional changes (points 1.4 and 2.1)
and pushed them to the branch.

The remaining points are still to be done. However, I'd like
to merge the branch into the master as it is and leave those points
for later.

[Ostap Cherkashin]

I went through the latest changes and it looks like you also fixed 2.2, 3.2 and 3.3. I think it is good to go into the master branch.

[Julius Chrobak]

Great news. I've merged the 'origin/params' branch to the master and
delete the remove branch.

On Aug 8, 6:57 pm, Ostap Cherkashin <ostap.cherkas...@gmail.com>
wrote: