Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

"count" aggregations fix #5639

Merged
merged 3 commits into from
Sep 10, 2024
Merged

"count" aggregations fix #5639

merged 3 commits into from
Sep 10, 2024

Conversation

YaroShkvorets
Copy link
Contributor

Fixes #5634

  • fix extra comma
  • add test case
  • refactor sql query construction to avoid same bug in other cases

@YaroShkvorets YaroShkvorets mentioned this pull request Sep 9, 2024
Closed
3 tasks
lutter
lutter requested changes Sep 9, 2024
View reviewed changes
Copy link
Collaborator

@lutter lutter left a comment

Choose a reason for hiding this comment

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

Nice catch! It seems that with these changes comma_sep is always called with first == false and we could therefore get rid of that argument and simplify the code a little.

@YaroShkvorets
Copy link
Contributor Author

Yes, I'll drop it.

@YaroShkvorets
Copy link
Contributor Author

Should be good now.

@YaroShkvorets YaroShkvorets requested a review from lutter September 9, 2024 17:32
lutter
lutter approved these changes Sep 9, 2024
View reviewed changes
Copy link
Collaborator

@lutter lutter 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 the PR!

@matthewdarwin
Copy link

@lutter can you merge it as well? @YaroShkvorets doesn't have merge permissions.

@lutter lutter merged commit c5640b1 into graphprotocol:master Sep 10, 2024
7 checks passed
@lutter
Copy link
Collaborator

lutter commented Sep 10, 2024

@lutter can you merge it as well? @YaroShkvorets doesn't have merge permissions.

Woops .. sorry, overlooked that. Merged now.

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@lutter lutter lutter approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Single "count" aggregation fails
3 participants
@YaroShkvorets @matthewdarwin @lutter