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

feat(alerts): apply SQL limit to all alerts #13150

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Feb 16, 2021

SUMMARY

Applies a limit of 2 to all alerts. Running huge queries on the DB by mistake may cause performance impact.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@codecov-io
Copy link

codecov-io commented Feb 16, 2021

Codecov Report

Merging #13150 (83b84f5) into master (2ce7982) will increase coverage by 10.95%.
The diff coverage is 57.31%.

Impacted file tree graph

@@             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     
Flag Coverage Δ
cypress 58.40% <70.92%> (+5.34%) ⬆️
python 67.07% <51.74%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...end/src/SqlLab/components/RunQueryActionButton.tsx 52.77% <ø> (ø)
superset-frontend/src/chart/ChartContainer.jsx 100.00% <ø> (ø)
superset-frontend/src/chart/ChartRenderer.jsx 75.67% <0.00%> (-1.04%) ⬇️
...perset-frontend/src/common/components/Dropdown.tsx 50.00% <ø> (ø)
...rontend/src/components/ListView/CardSortSelect.tsx 78.94% <ø> (ø)
...ontend/src/components/ListViewCard/ImageLoader.tsx 75.00% <0.00%> (ø)
...set-frontend/src/components/URLShortLinkButton.jsx 100.00% <ø> (ø)
...rset-frontend/src/components/URLShortLinkModal.tsx 77.77% <ø> (ø)
...src/dashboard/components/FiltersBadge/selectors.ts 86.07% <ø> (-3.93%) ⬇️
...src/dashboard/components/HeaderActionsDropdown.jsx 68.49% <ø> (+0.92%) ⬆️
... and 627 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8c32b8...83b84f5. Read the comment docs.

@@ -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
Copy link
Member

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.

Copy link
Member Author

@dpgaspar dpgaspar Feb 17, 2021

Choose a reason for hiding this comment

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

Very good idea!

@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/L 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants