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: Don't consolidate aggregated and non-aggregated queries without a groupby #479

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

andyrooger
Copy link
Contributor

In the QueryConsolidator, make sure SELECT x FROM table is not consolidated with SELECT count() FROM table since the aggregation in the latter query implies an implicit grouping which the former doesn't.

Implemented the fix by adding a GROUP BY * to the query's consolidation key where aggregates are present in the select.

Fixes #478

@andyrooger andyrooger force-pushed the 478-consolidator-top-agg branch from 0fc6da9 to 2bd15ab Compare August 13, 2024 11:15
@jheer
Copy link
Member

jheer commented Aug 13, 2024

Thank you for the PR! I also ran into this issue earlier when issuing field info summary queries. I added a fix specific to those at field-info.js#L41. I stopped short of the more general fix because I wanted to think more about cases where clients may pass in SQL strings instead of using the Mosaic query builders.

That said, I think a partial fix is better than none, and I appreciate the accompanying unit tests! I will review/merge when I have time to prep the next Mosaic release.

@andyrooger
Copy link
Contributor Author

Thanks @jheer . Would you like me to switch the GROUP BY * in the PR to use GROUP BY ALL to match the code you mentioned? Seems to be a better fit like that.

@jheer jheer merged commit 2bd15ab into uwdata:main Aug 22, 2024
2 checks passed
@jheer
Copy link
Member

jheer commented Aug 22, 2024

Thanks! I added the ALL (vs. *), added a comment and made the formatting the consistent.

@jheer jheer mentioned this pull request Sep 16, 2024
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.

Query consolidator can break when queries have non-grouped aggregations
2 participants