Alerts aren't firing with error 'Alert query returned a non-number value.' when returning a date column

[sadpandajoe]

A clear and concise description of what the bug is.

How to reproduce the bug

1. Create an alert
2. Use the examples database
3. Use this query: SELECT year FROM main.wb_health_population LIMIT 1
4. Set the condition as !=
5. Set the value a 0
6. Finish filling out the rest of the alert form

Expected results

The alert is sent because the condition is met

Actual results

Send error with Alert query returned a non-number value.

Screenshots

If applicable, add screenshots to help explain your problem.

Environment

(please complete the following information):

• browser type and version:
• superset version: superset version
• python version: python --version
• node.js version: node -v
• any feature flags active:

Checklist

Make sure to follow these steps before submitting your issue - thank you!

• I have checked the superset logs for python stacktraces and included it here as text if there are any.
• I have reproduced the issue with at least the latest released version of superset.
• I have checked the issue tracker for the same issue and I haven't found one similar.

Additional context

Looks like the first failure was with this SHA 2ac28927a326fee6431b5e01f7930e650c636c17

[michael-s-molina]

@sadpandajoe @eschutho @betodealmeida @john-bodley

Given that the threshold needs to be a double precision number, do the query always need to return a numeric value?

If that's the case, are we missing a tooltip on the SQL input explaining this constraint? I'm assuming the execution log will contain the error when this constraint is not respected.

[sadpandajoe]

This was working beforehand with a single digit so if we need double precision, it could potentially break for other people who don't currently have it.

[michael-s-molina]

This was working beforehand with a single digit so if we need double precision, it could potentially break for other people who don't currently have it.

@sadpandajoe I believe the error is not related to the double precision but to the fact that the query is returning a non-numeric value.

[john-bodley]

Per here it seems like year is a DATETIME column and thus shouldn't the specified alert query,

SELECT year FROM main.wb_health_population LIMIT 1

be (assuming SQLite):

SELECT CAST(strftime('%Y', year) AS INT) FROM main.wb_health_population LIMIT 1

[eschutho]

@sadpandajoe found that this is a regression from #26187. We bumped Pyarrow from 12.x to 14.x and there was a breaking change that impacts the value of year from an int (or other numerical value) to a timestamp, thus breaking existing alerts if someone is (probably incorrectly) returning the year as an int. If we want to maintain our own backward compatibility, we could cast it back to an int somewhere in the codebase. I agree with @john-bodley that the alert query should probably have included an explicit casting of the value to an int instead of relying on Pyarrow for the conversion. I'm not sure all of the places where this change has impacted the codebase other than alerts, or if we want to consider this a bug fix rather than a breaking change. Thoughts?

[john-bodley]

For context here's the line where @sadpandajoe's error was coming from. The Python float() method will try to convert any type into a float and will throw an exception otherwise:

>>> float('2023')
2023.0

>>> float('2023-12-01')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: could not convert string to float: '2023-12-01'
>>> 

[john-bodley]

@eschutho and @sadpandajoe are we sure this is a regression as I'm able to repro this using version 12.0.0 of pyarrow?

Additionally, per #26298 (comment), it seems like the year column is a DATETIME rather than an INT:

[john-bodley]

I've also been able to repro the behavior—which seems correct—on 3.0.2:

> git checkout 3.0.2
Updating files: 100% (1401/1401), done.
Note: switching to '3.0.2'.
...
HEAD is now at 961eba6d97 chore: Updates CHANGELOG.md for 3.0.2 (rc2)

> rm -rf examples.db
> python3.9 -m pip install -e .
> FLASK_DEBUG=true superset load-examples
> FLASK_DEBUG=true superset run --port 8088

> sqlite3 examples.db 
SQLite version 3.41.1 2023-03-10 12:13:52
Enter ".help" for usage hints.
sqlite> .schema wb_health_population
CREATE TABLE wb_health_population (
	country_name VARCHAR(255), 
	country_code VARCHAR(3), 
	region VARCHAR(255), 
	year DATETIME
);

[michael-s-molina]

Reading the comments it seems this is not a bug but an expected error when the query returns something that is not numeric. It seems a follow up could be adding a tooltip to the SQL input to make this restriction more clear as I suggested here.

[sadpandajoe]

@john-bodley yeah and if you do an alert based on that and in the successful logs you get -315619200000000000 as a return value which will trigger the alert. So either it's been an existing bug within alerts and reports that just worked or it's a breaking change and we should at least put it in the breaking change log so if people do upgrade it won't break their alerts.

[michael-s-molina]

or it's a breaking change and we should at least put it in the breaking change log so if people do upgrade it won't break their alerts.

@sadpandajoe I personally don't view this as a breaking change but more like a fix. Previously, if someone had a query that didn't return a numeric value, the application was wrongfully converting the return value and firing alerts under a incorrect condition evaluation. What the behavior should be is the current one, where errors are thrown when the condition cannot be evaluated. Right now, there are alerts that are being fired under false pretenses and they should stop working, or in other words, they should break. This change will give users the opportunity to fix misleading alerts that previously went unnoticed.