Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query Builder where clause should support more operators #18

Open
quangdatv opened this issue Jun 2, 2016 · 2 comments
Open

Query Builder where clause should support more operators #18

quangdatv opened this issue Jun 2, 2016 · 2 comments

Comments

@quangdatv
Copy link
Contributor

quangdatv commented Jun 2, 2016

As I read from the code, currently, Instream.Query.Builder where clause only supports equal operator. It should support more operators like >, <, >=, <=

One idea is for each {key, value} pair in the where conditions map, the key can be a string with format "field[space]op" whereas op is either ">", "<", ">=" nor "<=". If the key is an atom or op is omitted, we can consider the operator is equal.

For example:

# SELECT * FROM some_measurement WHERE binary = 'foo' AND numeric >= 42 AND location = 'abc'
from("some_measurement")
|> where(%{ binary: "foo", "numeric >=": 42, "location": "abc" })
|> MyApp.MyConnection.query()

Query Builder should also support LIMIT and OFFSET clauses
For example:

# SELECT * FROM some_measurement WHERE binary = 'foo' LIMIT 15 OFFSET 78
from("some_measurement")
|> where(%{ binary: "foo"})
|> limit(15)
|> offset(78)
|> MyApp.MyConnection.query()

If you don't mind, I can create a PR for this

@mneudert
Copy link
Owner

mneudert commented Jun 2, 2016

The query builder definitely needs a bucket of love already! Or in a more honest wording: "a complete rewrite".

(Separating blocks with some horizontal rules because long text ahead ^^)


First of all the thing that is definitely possible right now: OFFSET and LIMIT.

While the official documentation does not explicitely state it there seems to be no problem to use just one of them. Just passing an OFFSET gives results, so I assume it is supported or at least "working".

If you want please go ahead and send a pull request for those 👍


The WHERE statement is a bit more complicated however.

Your proposal of using spaces to separate the operator from the field might be a bit unusual to wrap ones head around. It seems like quite the hack for the sake of keeping existing syntax. The whole builder thing is marked experimental so we could just break things with a warning and do it future proof as soon as possible.

Future proof meaning the support of the full InfluxQL spec I am aiming at (future...). Like all the stuff I just looked up in the documentation:

  • WHERE location !~ /./
  • WHERE time > now() - 7d
  • WHERE water_level + 2 > 11.9 (nice one because the space for operator separation would lead to wrong escaping in the current implementation)
  • WHERE (location =~ /.*y.*/ OR location =~ /.*m.*/) (the grouping / OR is of interest here)

That is at least the point I would like to get to at some time.

But how could we get there?


  1. Replace all functions in the QueryBuilder with macros.

  2. Replace the current WHERE syntax with a single string statement and also a list of string statements resulting in AND concatenation.

    Now working with macros we could deprecate the old syntax and have a compile time warning. Functions would only allow for runtime warnings and that seems bad practice. Relying on people to have a test suite covering all query constructions is nice but unfair if we could do it at compile time. Especially for deprecations.

    This is probably the point where we have the complete WHERE syntax available. Forcing escaping to the user, but still complete syntax and all the opportunities!

  3. Add AST style conditions.

    Something like this would be really nice:

    |> where(field == ^variable)    # pin syntax to inject values
    |> where(time > now() - 7d)     # no variables found here!
    |> where(location = 'here' OR location = 'there')   # grouping, OR, ...

    That should be possible when working with macros. Perhaps not that easy but possible and imho the way to go in the long run.


Well, I probably should start working on those macros already...

@quangdatv
Copy link
Contributor Author

This is my PR: #19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants