influxdata / influxdb-client-csharp

InfluxDB 2.x C# Client
https://influxdata.github.io/influxdb-client-csharp/api/InfluxDB.Client.html
MIT License
354 stars 94 forks source link

Flux query builder fluent API #187

Open empz opened 3 years ago

empz commented 3 years ago

Currently, the queries have to be built manually in a string. It would be great to have a fluent API to build a query.

I know you guys are working on another library to query by using LINQ, but that requires having the data modeled and annotated in C# classes, which is not always possible due to the nature of dynamic data and metrics.

I'm proposing an API that can do things like the following:

var query = influxDb.GetQueryBuilder()
    .From("bucket")
    .Filter(r => r["_measurement"] == "cpu")
    .Filter(r => r["_field"] == "temperature")
    .Filter(r => r["some-tag"] == "some-value")
    .AggregateWindow(every: TimeSpan.FromSeconds(10), fn: InfluxDb.Functions.Mean, createEmpty: false)
    .Yield("mean")
    .ToString();

This way we could be writing queries in a .NET idiomatic way and ensuring the resulting strings are valid Flux queries.

bednar commented 3 years ago

@empz, thanks for the suggestion 👍

We don't have any plan to implement this feature, but every PR is welcome. Is this something you might be willing to help with?

empz commented 3 years ago

I'm very new at InfluxDb (and Flux) so I'm not sure I'm the right guy, but I could start something.

ldematte commented 3 years ago

I was just coming here to make the same request :) so well done @empz! I like the proposed API very much. I am actually using something very similar ATM. It should not be difficult to implement, keep us updated as I'm looking forward to use it!

empz commented 3 years ago

@ldematte Would you be willing to share your current approach? Maybe we can start building from it.

ldematte commented 3 years ago

It is extremely simple, and covers only my use cases. I am also new to Flux, that's why I built it only to suit my cases - I don't know if it can cover everything. Here is the basic structure (slightly modified to use an indexer as per-your suggestion)

public interface IInfluxDbFilterBuilder
    {
        string this[string index] { set; }
    }

    public class InfluxDbAggregateFunction
    {
        private readonly string m_functionName;

        private InfluxDbAggregateFunction(string functionName)
        {
            m_functionName = functionName;
        }

        public static InfluxDbAggregateFunction Mean = new InfluxDbAggregateFunction("mean");

        public override string ToString() => m_functionName;
    }

    public interface IInfluxDbQueryBuilder
    {
        IInfluxDbQueryBuilder From(string bucketName);

        IInfluxDbQueryBuilder Filter(Action<IInfluxDbFilterBuilder> buildFilter);

        IInfluxDbQueryBuilder AggregateWindow(TimeSpan every, InfluxDbAggregateFunction fn, bool createEmpty);

        string Build();
    }

In the implementation, each method just register its values in fields, then it is just a StringBuilder (+ a string.Join on the filters). No checks are done, you can produce invalid flux syntax, I think they should be added for a public use.

ldematte commented 3 years ago

What I am doing right now (well, tomorrow, or next time I touch this code) it is write a robust enough TimeSpan to duration converter, for AggregateWindow and Range.

That's exactly why I came here: I was wondering if it is already present in the code base (maybe in the Linq part?). I am happy to write it myself but I hate to reinvent the wheel!

bednar commented 3 years ago

@ldematte, @empz thanks a lot for your effort I will do my best to help you.

That's exactly why I came here: I was wondering if it is already present in the code base (maybe in the Linq part?).

Currently we don't have a converter from TimeSpan to duration.

.

I have just one note how the fluent builder API should produce the query. To builder should construct AST instead of string building the Flux queries. String building Flux queries opens the door to possible injection attacks from user input.

The values in fields should be replaced by identifier - something like v1 and the value of the field should be pass via Flux AST.

The output from API will be Query ... something like:

const string flux = "from(bucket: v1) |> ... |> limit(n: v2)";
var variables = new Dictionary<string, object>()
{
    {"v1", "my-bucket"},
    {"v2", 5},
};
var query = new Query(BuildAST(variables), flux);

Here are links how it is done in LINQ:

Feel free to refactor what you want to achieve reusability between LINQ and fluent query builder.

ldematte commented 3 years ago

@bednar I agree 100%, that's why I did not want to integrate/propose my simple builder, which was implemented with a "works with my code" attitude :) Since there are already classes for an AST makes sense using them too. After a 2-min look probably the best way is to refactor the existing code by decoupling the visitor part (maybe injectable through an interface?) so that the visitor can start from a LINQ expression tree (today code) or from data collected from the builder. I would still go with something like @empz proposal/the prototype interface above, but instead of building a string (ToString or string Build()) return a Query (Query Build()).

What do you guys think?

I am quite busy with another "free time" project ATM, but I could tackle this next if you guys want me to. Of course if @empz decides to go on and give it a go on its own, I'm happy to let him grab this issue! 👍

bednar commented 3 years ago

@bednar I agree 100%, that's why I did not want to integrate/propose my simple builder, which was implemented with a "works with my code" attitude :) Since there are already classes for an AST makes sense using them too. After a 2-min look probably the best way is to refactor the existing code by decoupling the visitor part (maybe injectable through an interface?) so that the visitor can start from a LINQ expression tree (today code) or from data collected from the builder.

👍 it sounds good

I would still go with something like @empz proposal/the prototype interface above, but instead of building a string (ToString or string Build()) return a Query (Query Build()). What do you guys think?

I agree. The API looks pretty useful.

var query = influxDb.GetQueryBuilder()
    .From("bucket")
    .Filter(r => r["_measurement"] == "cpu")
    .Filter(r => r["_field"] == "temperature")
    .Filter(r => r["some-tag"] == "some-value")
    .AggregateWindow(every: TimeSpan.FromSeconds(10), fn: InfluxDb.Functions.Mean, createEmpty: false)
    .Yield("mean")
    .ToString();

I am quite busy with another "free time" project ATM, but I could tackle this next if you guys want me to. Of course if @empz decides to go on and give it a go on its own, I'm happy to let him grab this issue! 👍

I can prepare new branch for the Fluent query builder , but unfortunately the GitHub doesn't supports to grant permission to write only for a specified branch. You will have to do PRs into this branch 😢.

empz commented 3 years ago

I'm not very experienced at building ASTs. I see the LINQ implementation uses the Remotion.Linq under the hood.

Does anybody know if there's something that helps you build AST in C#? I'd hate to re-invent the wheel too. Especially if I'm not an expert in the topic.

ldematte commented 3 years ago

Building an AST from a builder should be quite easy, as you can just create the nodes of the tree directly from the builder calls - no need for external libraries, and no need to build a parser! You basically have already the user giving the structure of the tree through the calls. In the end, an AST is simply a tree data structure of different classes (all inherited from a base SyntaxNode class).

Example: the query itself could be represented by a Root node, with a From children (which will be a leaf probably), that will be populated by the .From() call, + N Operation children, each populated by a .Aggregate(), .Filter(), .Range() ecc. call.... you see were I am going. At each call you create a node (of a correct class) and insert it in the appropriate position in the tree.

Filter would be the most interesting case, as you probably want the expression to be a sub-tree of a Filter node (to do checks, control the structure, perform substitutions, ecc.),. You can keep the API as it is, accumulate the intermediate nodes in a temp data structure and then aggregate them under a node during the Build call, or you could end up changing the API a bit; it is also the place where you want to do the variable substitution @bednar mentioned, but a dictionary in the builder should be a good enough "symbol table" for this case.

ldematte commented 3 years ago

The interesting bit is, IMO, the AST visitor part, because since there is already the LINQ implementation (from Linq.Expression nodes), it is were there should be an opportunity to refactor and reuse.

bednar commented 3 years ago

I'm not very experienced at building ASTs. I see the LINQ implementation uses the Remotion.Linq under the hood.

Does anybody know if there's something that helps you build AST in C#? I'd hate to re-invent the wheel too. Especially if I'm not an expert in the topic.

@empz, Sorry for ambiguities... for the Fluent Query Builder we will have to use Flux AST for parameter assignments not for the builder.

xljiulang commented 3 years ago

I've already done it: Influxdb2.Client

ldematte commented 3 years ago

I've already done it: Influxdb2.Client

Hi! Do you plan to port it to this repository and submit a pull request? Otherwise I would say this issue should stay open!

empz commented 3 years ago

I've already done it: Influxdb2.Client

Yes, but that uses a StringBuilder to build the query which is what the team here is trying to avoid. I think that in order for that library to be merged into this repo it would need to make use of the Flux AST present in this library.

epustovit commented 3 years ago

Hi, guys. Is there any branch for a query builder where I could push the changes? @bednar

bednar commented 3 years ago

@epustovit feat/query-builder

epustovit commented 3 years ago

What if I use QueryAggregator wrapped in the new IInfluxDbQueryBuilder? And when user calls .Build(), it creates visitor, which takes QueryAggregator and VariableAggregator.

bednar commented 3 years ago

What if I use QueryAggregator wrapped in the new IInfluxDbQueryBuilder? And when user calls .Build(), it creates visitor, which takes QueryAggregator and VariableAggregator.

Sounds good 👍

MalikRizwanBashir commented 3 years ago

Guys i have written a basic fluent implementation for Flux query builder Have a look! https://github.com/MalikRizwanBashir/FluxQuery.Net

jobaeurle commented 1 year ago

Is this still in doing? we could use something similar, best for us would actually be a typed linq syntax but not with the current queryable but one that just returns the query so we can use it with the IQueryApi.Query. Because then we would be able to unit test all the stuff because we cannot mock the static Queryable method...

danpercic86 commented 5 months ago

Hey, do you plan to add this sometime?

bednar commented 5 months ago

Hi @danpercic86,

Thank you for bringing this to our attention. At the moment, this item is not prioritized in our backlog.

Would you be interested in contributing to this project by working on this feature yourself? We welcome Pull Requests (PRs) from our community and are more than happy to review your submission. Your involvement could greatly accelerate the addition of this feature to the project.

If you decide to contribute, please let us know if you need any guidance or information to get started. We're here to support you in any way we can.

Best regards.