-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
feat(alerts): apply SQL limit to all alerts #13150
feat(alerts): apply SQL limit to all alerts #13150
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13150 +/- ##
===========================================
+ Coverage 53.06% 64.01% +10.95%
===========================================
Files 489 960 +471
Lines 17314 44815 +27501
Branches 4482 4072 -410
===========================================
+ Hits 9187 28690 +19503
- Misses 8127 16125 +7998
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
superset/reports/commands/alert.py
Outdated
@@ -117,7 +117,10 @@ def validate(self) -> None: | |||
) | |||
rendered_sql = sql_template.process_template(self._report_schedule.sql) | |||
try: | |||
df = self._report_schedule.database.get_df(rendered_sql) | |||
limited_rendered_sql = self._report_schedule.database.apply_limit_to_sql( | |||
rendered_sql, 1 |
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.
I'm wondering if we should set the limit to 2 - the system expects a single row and raises an error if multiple rows are returned. This error could surface an issue with the query, or the data being accessed. Setting the limit to 1 changes the behavior of the query in a way that could hide issues.
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.
Very good idea!
SUMMARY
Applies a limit of 2 to all alerts. Running huge queries on the DB by mistake may cause performance impact.
ADDITIONAL INFORMATION