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

fix: apply having and limit clause on formulas #5201

Merged
merged 3 commits into from
Jun 13, 2024
Merged

Conversation

srikanthccv
Copy link
Member

Summary

Fixes #5103

@github-actions github-actions bot added the bug Something isn't working label Jun 12, 2024
@srikanthccv srikanthccv marked this pull request as ready for review June 13, 2024 04:58
Copy link
Member

@nityanandagohain nityanandagohain left a comment

Choose a reason for hiding this comment

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

changes looks good to me, but this also means that we should remove having from the SQL queries for builder queries right ?

I had one query though if there are two queries, A & B, and A has some having clause.

Now I have a formula F(A,B) with some having clause, should the having clause of A be taken into account, yes right ?

Also this changes mean that we can remove having for SQL queries to clickhouse right as it will be done in postprocess ?

@srikanthccv
Copy link
Member Author

but this also means that we should remove having from the SQL queries for builder queries right ?

For v3, it doesn't make any difference because applying having again gives the same result. In v4, the formula query is evaluated using postprocess so there is nothing to remove.

Now I have a formula F(A,B) with some having clause, should the having clause of A be taken into account, yes right ?

Yes

Also this changes mean that we can remove having for SQL queries to clickhouse right as it will be done in postprocess ?

Technically yes, and it's better because it will be cache effective. You can remove this

if len(query.Having) > 0 {
for idx, having := range query.Having {
parts = append(parts, fmt.Sprintf("having-%d=%s", idx, having.CacheKey()))
}
}
. The having posts aggregation filter. For example, if there are two same queries but different having. Today logs use the two different cache entries but there is no need to do that. Just use the one cache item and run the having part in query service.

@srikanthccv srikanthccv merged commit cacf4b9 into develop Jun 13, 2024
12 checks passed
@srikanthccv srikanthccv deleted the fix-post-process branch June 13, 2024 15:07
@nityanandagohain
Copy link
Member

Sure creating an issue for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OrderBy and Limit does not work in Formula in dashboard
2 participants