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

Add support for dynamic objects in Aggregate and Group by fields #390

Merged
merged 3 commits into from Jun 1, 2022

Conversation

andresmgot
Copy link
Contributor

I noticed that dynamic values cannot be used for the summarize function (used for the Aggregate and Group by fields). For aggregating, I am converting the value to a long since its goal is to be used with a mathematical function. For grouping I am converting the value to a string. Note that the goal of this is not to support all the possible use cases but to at least give valid syntax:

Aggregate example:
Screenshot from 2022-06-01 11-18-41

Group by example:
Screenshot from 2022-06-01 11-19-07

I tried to do some unit test for this but it seems that the current components are not working with the react testing library (clicking on the "+" button didn't render the selector). I could do some e2e test for covering this functionality but it doesn't seem worth it. Let me know what you think.

@andresmgot andresmgot requested a review from aangelisc June 1, 2022 09:25
@andresmgot andresmgot requested a review from a team as a code owner June 1, 2022 09:25
@andresmgot andresmgot requested review from kevinwcyu and removed request for a team June 1, 2022 09:25
@github-actions
Copy link

github-actions bot commented Jun 1, 2022

Code coverage report for PR #390

Go TypeScript
main 54.5% 60.86%
PR 54.5% 62.81%
difference 0% 1.95%

Copy link
Contributor

@aangelisc aangelisc left a comment

Choose a reason for hiding this comment

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

LGTM! Just a minor question.

I'm happy with leaving the e2e test out as I agree it will be excess work for little gain. I think opening a separate ticket to investigate why the unit tests don't work as expected is probably a better course of action. Once that's solved we can unit test accordingly 👍

@andresmgot
Copy link
Contributor Author

I think opening a separate ticket to investigate why the unit tests don't work as expected is probably a better course of action. Once that's solved we can unit test accordingly

I think we should actually reduce the amount of time invested in supporting the current query builder. Instead, we should try to focus our efforts on using a generic query builder, like the one that is exposed now in the @grafana/experimental package. I have created a new issue / epic to track this effort: #391

@andresmgot andresmgot merged commit 260e4b0 into main Jun 1, 2022
@andresmgot andresmgot deleted the aggregateObj branch June 1, 2022 11:04
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

Successfully merging this pull request may close these issues.

None yet

2 participants