-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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: new reports scheduler #11711
feat: new reports scheduler #11711
Changes from 15 commits
bb7dea9
136d4ae
b352280
d2f0d05
cfa92eb
2ff6de2
0e1f081
198cd75
91751aa
260838b
32e40e3
f78dd9e
44923b4
89abae3
094a3c7
c5ccd12
d0e3770
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
# Licensed to the Apache Software Foundation (ASF) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The ASF licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
import json | ||
import logging | ||
from operator import eq, ge, gt, le, lt, ne | ||
from typing import Optional | ||
|
||
import numpy as np | ||
|
||
from superset import jinja_context | ||
from superset.commands.base import BaseCommand | ||
from superset.models.reports import ReportSchedule, ReportScheduleValidatorType | ||
from superset.reports.commands.exceptions import ( | ||
AlertQueryInvalidTypeError, | ||
AlertQueryMultipleColumnsError, | ||
AlertQueryMultipleRowsError, | ||
) | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
OPERATOR_FUNCTIONS = {">=": ge, ">": gt, "<=": le, "<": lt, "==": eq, "!=": ne} | ||
|
||
|
||
class AlertCommand(BaseCommand): | ||
def __init__(self, report_schedule: ReportSchedule): | ||
self._report_schedule = report_schedule | ||
self._result: Optional[float] = None | ||
|
||
def run(self) -> bool: | ||
self.validate() | ||
|
||
if self._report_schedule.validator_type == ReportScheduleValidatorType.NOT_NULL: | ||
self._report_schedule.last_value_row_json = self._result | ||
return self._result not in (0, None, np.nan) | ||
self._report_schedule.last_value = self._result | ||
operator = json.loads(self._report_schedule.validator_config_json)["op"] | ||
threshold = json.loads(self._report_schedule.validator_config_json)["threshold"] | ||
return OPERATOR_FUNCTIONS[operator](self._result, threshold) | ||
|
||
def validate(self) -> None: | ||
""" | ||
Validate the query result as a Pandas DataFrame | ||
""" | ||
sql_template = jinja_context.get_template_processor( | ||
database=self._report_schedule.database | ||
) | ||
rendered_sql = sql_template.process_template(self._report_schedule.sql) | ||
df = self._report_schedule.database.get_df(rendered_sql) | ||
|
||
if df.empty: | ||
return | ||
rows = df.to_records() | ||
if self._report_schedule.validator_type == ReportScheduleValidatorType.NOT_NULL: | ||
self._result = rows[0][1] | ||
return | ||
# check if query return more then one row | ||
if len(rows) > 1: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it will be useful to add result to the exception & surface it to the user in the error msg There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about surfacing the number of returned rows? Would be nice to try to wrap all the rows in a string, but need to be careful and truncate it to a certain point, think about a user mistake with millions of rows? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, number of rows is a good start, we can tune it later, your right that it needs smart truncation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, done that |
||
raise AlertQueryMultipleRowsError() | ||
dpgaspar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if len(rows[0]) > 2: | ||
raise AlertQueryMultipleColumnsError() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we only want to raise here if the validator is something other than "NOT NULL" - if a row is returned, then it's not null :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ups! Right you are There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: it may be cleaner to have separate validation functions depending on the type of the validator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
if rows[0][1] is None: | ||
return | ||
try: | ||
# Check if it's float or if we can convert it | ||
self._result = float(rows[0][1]) | ||
return | ||
except (AssertionError, TypeError, ValueError): | ||
raise AlertQueryInvalidTypeError() |
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 think this is correct if the result is a float, but if it's a row or set of rows this should behave differently.