-
Notifications
You must be signed in to change notification settings - Fork 14k
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(sqla): avoid unnecessary groupby in samples request #18579
Conversation
Codecov Report
@@ Coverage Diff @@
## master #18579 +/- ##
==========================================
- Coverage 66.29% 66.29% -0.01%
==========================================
Files 1594 1594
Lines 62623 62623
Branches 6312 6312
==========================================
- Hits 41518 41516 -2
- Misses 19456 19458 +2
Partials 1649 1649
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ! nice to feel the value of chart data separated test suite!
Agreed, the chart data test suite is a great tool to help harden the poorly tested sqla connector 👍 |
SUMMARY
When requesting samples from a dataset, a
GROUP BY
was incorrectly added to the query due to themetrics
parameter in theQueryObject
being set to an empty list. This adds unnecessary computational expense to the query and doesn't reflect the raw data correctly by hiding potential duplicates in the underlying data. This PR fixes the bug, adds assertions to existing tests to ensure the sample query is executed without aGROUP BY
clause and adds instructions to the API spec on what the difference is between anull
vs empty array for themetrics
parameter.TESTING INSTRUCTIONS
GROUP BY
clause.ADDITIONAL INFORMATION