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

feat: support string functions [LINQ] #500

Closed
wants to merge 1 commit into from

Conversation

jogibear9988
Copy link

@jogibear9988 jogibear9988 commented Apr 3, 2023

Closes #499

Proposed Changes

Support string functions from linq

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • dotnet test completes successfully
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@jogibear9988
Copy link
Author

@powersj could you review?

@jogibear9988
Copy link
Author

@bednar could you review this?

@bednar bednar self-requested a review May 4, 2023 06:34
@bednar
Copy link
Contributor

bednar commented May 4, 2023

@bednar could you review this?

Yes, I can. Can you rebase sources to trigger checks?

Regards

@jogibear9988
Copy link
Author

rebase done

@jogibear9988
Copy link
Author

@bednar an chance to get this merged?

@bednar
Copy link
Contributor

bednar commented Jul 13, 2023

Hi @jogibear9988,

I plan to review your PR either today or tomorrow. In the meantime, could you please correct the formatting of your code? You can do this by running ./Scripts/reformat-code.sh in the root of the project.

Best Regards

@bednar bednar changed the title first draft of influx string functions, fixes #499 feat: support string functions [LINQ] Jul 13, 2023
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR 👍

There are a few requirements that must be be satisfy before we accept the PR:

  1. Please update CHANGELOG.md file

and also:

Client.Linq/Internal/QueryExpressionTreeVisitor.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/QueryExpressionTreeVisitor.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/QueryExpressionTreeVisitor.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/QueryExpressionTreeVisitor.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/QueryExpressionTreeVisitor.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/QueryExpressionTreeVisitor.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/QueryExpressionTreeVisitor.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/QueryAggregator.cs Show resolved Hide resolved
Client.Linq/Internal/QueryAggregator.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/QueryAggregator.cs Show resolved Hide resolved
@jogibear9988
Copy link
Author

I've now created the classes, but with the name of the C# functions. Is this okay? I could also use the name of Influx, but then also "ToString" should be named "String"

Client.Linq/Internal/Expressions/String/Contains.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/Expressions/String/EndsWith.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/Expressions/String/Replace.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/Expressions/String/Replace.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/Expressions/String/StartsWith.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/Expressions/String/ToLower.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/Expressions/String/ToString.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/Expressions/String/ToUpper.cs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Client.Linq/Internal/Expressions/String/Contains.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/Expressions/String/EndsWith.cs Outdated Show resolved Hide resolved
Client.Linq/Internal/Expressions/String/StartsWith.cs Outdated Show resolved Hide resolved
@jogibear9988
Copy link
Author

Should I change QueryAggregator, to use Lazy initalized Fields for the other parts as well? So only memory is used when for example filters, sortings,... are used?

@bednar
Copy link
Contributor

bednar commented Jul 17, 2023

I've now created the classes, but with the name of the C# functions. Is this okay? I could also use the name of Influx, but then also "ToString" should be named "String"

It is ok. Thanks

Should I change QueryAggregator, to use Lazy initalized Fields for the other parts as well? So only memory is used when for example filters, sortings,... are used?

If you have the time, yes, please make the change. However, if you're unable to do so, you may leave the code as is; it won't block the approval process.

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we're almost ready to merge this PR into our master branch. Please satisfy the following requirements for the tests:

Comment on lines +41 to +82
[Test]
public void StringFunctionsQuery()
{
var query = from s in InfluxDBQueryable<Sensor>.Queryable("my-bucket", "my-org", _queryApi)
where s.SensorId.ToLower().Contains("aaa")
select s;
var visitor = BuildQueryVisitor(query);

const string expected =
"import \"strings\"\nstart_shifted = int(v: time(v: p2))\n\nfrom(bucket: p1) |> range(start: time(v: start_shifted)) |> filter(fn: (r) => strings.containsStr(v: strings.toLower(v: r[\"sensor_id\"]), substr: p3)) |> pivot(rowKey:[\"_time\"], columnKey: [\"_field\"], valueColumn: \"_value\") |> drop(columns: [\"_start\", \"_stop\", \"_measurement\"]) |> filter(fn: (r) => strings.containsStr(v: strings.toLower(v: r[\"sensor_id\"]), substr: p3))";
var qry = visitor.BuildFluxQuery();
Assert.AreEqual(expected, visitor.BuildFluxQuery());
}

[Test]
public void ToStringFunctionQuery()
{
var query = from s in InfluxDBQueryable<Sensor>.Queryable("my-bucket", "my-org", _queryApi)
where s.Value.ToString() == "3"
select s;
var visitor = BuildQueryVisitor(query);

const string expected =
"start_shifted = int(v: time(v: p2))\n\nfrom(bucket: p1) |> range(start: time(v: start_shifted)) |> filter(fn: (r) => (string(v: r[\"data\"]) == p3)) |> pivot(rowKey:[\"_time\"], columnKey: [\"_field\"], valueColumn: \"_value\") |> drop(columns: [\"_start\", \"_stop\", \"_measurement\"]) |> filter(fn: (r) => (string(v: r[\"data\"]) == p3))";
var qry = visitor.BuildFluxQuery();
Assert.AreEqual(expected, visitor.BuildFluxQuery());
}

[Test]
public void ReplaceAllFunctionQuery()
{
var query = from s in InfluxDBQueryable<Sensor>.Queryable("my-bucket", "my-org", _queryApi)
where s.SensorId.ToLower().Replace("a", "b") == "b"
select s;
var visitor = BuildQueryVisitor(query);

const string expected =
"import \"strings\"\nstart_shifted = int(v: time(v: p2))\n\nfrom(bucket: p1) |> range(start: time(v: start_shifted)) |> filter(fn: (r) => (strings.replaceAll(v: strings.toLower(v: r[\"sensor_id\"]), t: p3, u: p4) == p5)) |> pivot(rowKey:[\"_time\"], columnKey: [\"_field\"], valueColumn: \"_value\") |> drop(columns: [\"_start\", \"_stop\", \"_measurement\"]) |> filter(fn: (r) => (strings.replaceAll(v: strings.toLower(v: r[\"sensor_id\"]), t: p3, u: p4) == p5))";
var qry = visitor.BuildFluxQuery();
Assert.AreEqual(expected, visitor.BuildFluxQuery());
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include tests for all new string functions, and also evaluate the parameters of the Flux query. Here's an example of what we're looking for:

Assert.AreEqual("4", GetLiteral<IntegerLiteral>(ast, 2).Value);

@jogibear9988
Copy link
Author

hopefully i can fix the rest next week

@bednar
Copy link
Contributor

bednar commented Jul 31, 2023

@jogibear9988 thanks, awesome news 👍

@jogibear9988
Copy link
Author

@bednar
hey, I think I'll have no time to fix this in the next time. all the functions work, but as influx will not support flux official in the next version, I'll think I'll leave it as is.

@bednar
Copy link
Contributor

bednar commented Nov 1, 2023

This PR has been closed because it has not had recent activity. Please reopen if this PR is still important to you and you want to continue with them.

@bednar bednar closed this Nov 1, 2023
@bednar bednar added this to the 4.14.0 milestone Nov 1, 2023
@bednar bednar added invalid This doesn't seem right wontfix This will not be worked on labels Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Influx Linq Provider seems not support string functions
3 participants