persvr / rql

Resource Query Language
http://www.persvr.org/
267 stars 49 forks source link

Wrong identifier #71

Open ozum opened 8 years ago

ozum commented 8 years ago

Hi,

sum(price)=gt=3 query generates following object:

{
    name: 'and',
    args: [
        { name: 'sumgt', args: [ [ 'price' ], 3 ] }
    ],
    cache: {}
}

Shouldn't it be like gt(sum(price),3)

{
    name: 'and',
    args: [{
        name: 'gt',
        args: [ { name: 'sum', args: [ 'price' ] }, 3 ]
    } ],
    cache: {}
}

Or at least it may throw exception.

Best Regards,

wshager commented 8 years ago

This is not possible in RQL, please see issue #51. I'm working on a solution for this, and will post it soon.

ozum commented 8 years ago

Thank you for your response.

Is sum(price)=gt=3 not valid? If it is not valid then, throwing something similar to Invalid RQL exception would be great.

Otherwise users type sum(price)=gt=3 in URL and RQL parser parses this string as in my previous message. Then my library processes parsed tree and sees 'sumgt' and my library throws exception that 'sumgt' is not a function. Users get confused, because they didn't type 'sumgt'.

This seems simple in this example, but in more complex queries it becomes very hard to see what's wrong.

Thanks again,

wshager commented 8 years ago

@ozum more complex queries are possible in a Turing-complete query language, for instance xquery. AFAIK this was never the goal of RQL. Even if you could write the kind of queries you propose, at some point you'd want to create temporaries and the whole shebang (to not have to type in sum(price) multiple times)...

I don't think you should always expose the entire query to the user though, as that could indeed lead to unexpected results, and might even be considered unsafe in some cases. I usually create a mapping between URI fragments and complex queries. In other words: always provide users with a bunch of understandable links.

ozum commented 8 years ago

Thank you.

rkaw92 commented 8 years ago

I feel that the core issue here is that there are actually two kinds of "operators" in RQL: those that evaluate as a logical sentence for filtering entries in a collection (i.e. true/false predicates) and those that mutate the behaviour / output format of the retrieval in other ways. In my opinion, those should be always kept separate: it is probably pointless to perform a logical "or" on sorting or aggregation directives. Thus, I consider these "non-logical" operators as more of commands, really. To be fair, RQL bugs me a bit because it mixes the two in a single query string (and, in fact, under a common logical AND predicate).

Indeed, I find that there is an implicit parity between what MongoDB (lacking an expression language) can do, and what RQL may represent. Example: in MongoDB, you can do eq(field, constantValue), but not eq(field, field), nor eq(value, value), or even eq(field + 1, value). I suppose this is a desired feature of the language, though I have not seen it stated explicitly.

This may need clearing up in the README. A separation between things that count as predicates and those useful for sorting/limiting/aggregation etc. would also be welcome.

wshager commented 8 years ago

@rkaw92 I agree. Syntactically it would be correct to add a filter() function, which evaluates the filter. This is indeed how functions are evaluated, as anything but the filter operators is considered top-level.

I'd add this to the parser, but it would break compatibility, so instead I suggest adding a normalization that wraps operators in a filter(), so we can move towards a Turing-complete RQL.

ozum commented 8 years ago

Oh, I think I misrepresent my question, I try to rephrase it:

As my understanding of RQL documentation, those two queries are equal RQL queries:

sum(price)=gt=3 is identical to gt(sum(price),3)

I expect RQL parser generates identical AST for those equal queries, but RQL parser generates two different AST for those equal queries.

There are two possibilities: A. I'm wrong, those are not equal/identical queries. B. RQL parser generates wrong AST.

Best Regards,