Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: year 54714 is out of range issue with async queries #21656

Closed
wants to merge 2 commits into from

Conversation

hughhhh
Copy link
Member

@hughhhh hughhhh commented Sep 29, 2022

SUMMARY

Reverting PR causing issues with async queries
Whenever a user enables async queries, all results are throwing the following error:

Traceback (most recent call last):
File "/Users/hugh/src/superset/venv/lib/python3.8/site-packages/celery/app/trace.py", line 451, in trace_task
R = retval = fun(args, **kwargs)
File "/Users/hugh/src/superset/superset/initialization/init.py", line 105, in call
return task_base.call(self, *args, *kwargs)
File "/Users/hugh/src/superset/venv/lib/python3.8/site-packages/celery/app/trace.py", line 734, in __protected_call__
return self.run(*args, **kwargs)
File "/Users/hugh/src/superset/superset/sql_lab.py", line 189, in get_sql_results
raise ex
File "/Users/hugh/src/superset/superset/sql_lab.py", line 178, in get_sql_results
return execute_sql_statements(
File "/Users/hugh/src/superset/superset/sql_lab.py", line 511, in execute_sql_statements
raise ex
File "/Users/hugh/src/superset/superset/sql_lab.py", line 499, in execute_sql_statements
result_set = execute_sql_statement(
File "/Users/hugh/src/superset/superset/sql_lab.py", line 237, in execute_sql_statement
start_dttm = datetime.fromtimestamp(query.start_time)
ValueError: year 54714 is out of range

Screen Shot 2022-09-29 at 1 45 13 PM

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Merging #21656 (ecb3668) into master (4c17f0e) will not change coverage.
The diff coverage is 52.50%.

@@           Coverage Diff           @@
##           master   #21656   +/-   ##
=======================================
  Coverage   66.82%   66.82%           
=======================================
  Files        1798     1798           
  Lines       68827    68827           
  Branches     7333     7333           
=======================================
  Hits        45997    45997           
  Misses      20951    20951           
  Partials     1879     1879           
Flag Coverage Δ
hive 52.91% <ø> (ø)
mysql 78.23% <ø> (ø)
postgres 78.29% <ø> (ø)
presto 52.81% <ø> (ø)
python 81.42% <ø> (ø)
sqlite 76.79% <ø> (ø)
unit 50.92% <ø> (ø)

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

Impacted Files Coverage Δ
superset-frontend/src/SqlLab/constants.ts 100.00% <ø> (ø)
...frontend/src/SqlLab/components/SqlEditor/index.jsx 54.34% <52.50%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@exherb
Copy link

exherb commented Sep 30, 2022

why frontend change cause this issue?

@EugeneTorap
Copy link
Contributor

@villebro The same story like in #21298 PR.
Without the desire to figure out what logic exactly causes this error, fix only this error and cover it with tests so that there are no problems with this in the future, we revert my PR?

@zhaoyongjie zhaoyongjie self-requested a review September 30, 2022 09:03
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

Hi @hughhhh, you should check out the query.star_time from logging.

start_dttm = datetime.fromtimestamp(query.start_time)
ValueError: year 54714 is out of range

@hughhhh hughhhh closed this Sep 30, 2022
@hughhhh hughhhh reopened this Sep 30, 2022
@hughhhh
Copy link
Member Author

hughhhh commented Sep 30, 2022

@EugeneTorap i spent the majority of the week trying to figure out the issue, and actually update the logic. With no luck I was hoping to revert this code since it's actually blocking all async queries execution then we can revisit the issue from your PR.

The main thing is that master is broken because of this PR and is blocking our release of superset.

@exherb @zhaoyongjie for some reason with the conversion of the component the client is sending milliseconds instead of seconds, the weird thing is the code isn't touching anything with startDttm

@yousoph
Copy link
Member

yousoph commented Sep 30, 2022

/testenv up

@github-actions
Copy link
Contributor

@yousoph Ephemeral environment creation failed. Please check the Actions logs for details.

@justinpark
Copy link
Member

@hughhhh I found the root cause of this bug. Please see the PR #21667

cc: @EugeneTorap

@EugeneTorap
Copy link
Contributor

EugeneTorap commented Sep 30, 2022

This is great news, we don't need to revert my PR!

@hughhhh hughhhh closed this Oct 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2022

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch mistercrunch deleted the revert-2224ebecf branch March 26, 2024 16:20
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants