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

fix: Cast out of bounds timestamps as objects #14006

Closed
wants to merge 2 commits into from
Closed

fix: Cast out of bounds timestamps as objects #14006

wants to merge 2 commits into from

Conversation

cabo40
Copy link
Contributor

@cabo40 cabo40 commented Apr 7, 2021

SUMMARY

As a fix to #13661, timestamps can be passed as objects https://issues.apache.org/jira/browse/ARROW-7856

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

image
image

TEST PLAN

In a fresh installation set async query for the examples database and execute the next query in sql lab:

SELECT TIMESTAMP '01-01-1000';

ADDITIONAL INFORMATION

Fixes #13661

  • Has associated issue: [sql-lab]Pyarrow to pandas fails when timestamp is out of bounds #13661
  • 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 Apr 7, 2021

Codecov Report

Merging #14006 (6de8434) into master (72d19b6) will increase coverage by 1.87%.
The diff coverage is 82.87%.

❗ Current head 6de8434 differs from pull request most recent head adfacb2. Consider uploading reports for the commit adfacb2 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14006      +/-   ##
==========================================
+ Coverage   77.41%   79.28%   +1.87%     
==========================================
  Files         918      939      +21     
  Lines       46673    47541     +868     
  Branches     5720     5938     +218     
==========================================
+ Hits        36132    37694    +1562     
+ Misses      10405     9726     -679     
+ Partials      136      121      -15     
Flag Coverage Δ
cypress 56.04% <61.42%> (-0.56%) ⬇️
hive ?
mysql 80.71% <ø> (+0.15%) ⬆️
postgres 80.74% <ø> (+0.13%) ⬆️
presto ?
python 80.82% <ø> (-0.29%) ⬇️
sqlite 80.34% <ø> (+0.12%) ⬆️

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

Impacted Files Coverage Δ
...rset-frontend/src/SqlLab/components/QueryTable.jsx 66.66% <ø> (ø)
...erset-frontend/src/SqlLab/components/SqlEditor.jsx 56.01% <ø> (ø)
superset-frontend/src/chart/chartReducer.ts 66.19% <ø> (+1.81%) ⬆️
...end/src/common/components/DropdownButton/index.tsx 24.00% <0.00%> (ø)
...ontend/src/common/hooks/usePrevious/usePrevious.ts 100.00% <ø> (ø)
...rset-frontend/src/components/ErrorMessage/types.ts 100.00% <ø> (ø)
...perset-frontend/src/components/FormLabel/index.tsx 100.00% <ø> (ø)
superset-frontend/src/components/Loading/index.tsx 100.00% <ø> (ø)
...rset-frontend/src/components/ProgressBar/index.tsx 100.00% <ø> (ø)
...ontend/src/components/URLShortLinkButton/index.jsx 100.00% <ø> (ø)
... and 333 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 bd1d6ac...adfacb2. Read the comment docs.

@junlincc junlincc requested a review from zhaoyongjie April 8, 2021 17:34
@junlincc
Copy link
Member

junlincc commented Apr 8, 2021

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2021

@junlincc Ephemeral environment spinning up at http://54.202.136.119:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

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.

Thank you for this fix. LGTM! Just need to fix the pre-commit. please run the following command in your terminal.

$ tox -e pre-commit

@junlincc
Copy link
Member

@cabo40 almost good to go! can you update the PR description using the PR template, making sure to check the boxes that are relevant, also get the last check passed.

@cabo40
Copy link
Contributor Author

cabo40 commented Apr 14, 2021

Hi, thank you vm for reviewing this PR. I looked into the Translations test but could not find how to fix it or why it broke, sorry to bother with such triviality but, how can I get the test passed?

@zhaoyongjie
Copy link
Member

duplicate with #13830, close this PR.

@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@cabo40
Copy link
Contributor Author

cabo40 commented Apr 21, 2021

Hi @zhaoyongjie , sorry, the error persists after merging #13830. Can we open again this PR? The files changed in the PR are different. Also, the error is now this
Casting from timestamp[us] to timestamp[ns] would result in out of bounds timestamp: -30610224000000000
image

@cabo40
Copy link
Contributor Author

cabo40 commented Jun 15, 2021

Hi. Sorry, can we open this PR? It is not duplicate and issue #13661 persists

@simonvanderveldt
Copy link

simonvanderveldt commented Feb 22, 2022

@junlincc @zhaoyongjie Can this PR be reopened and merged? Superset is still broken with regards to this issue.

@villebro
Copy link
Member

@simonvanderveldt I am unable to repro on a Postgres db instance on master branch:

image

If this is still a problem, can you open a new issue with reproduction steps, along with detailed info about affected versions (Superset + Postgres db) so we can figure out what the problem is.

@simonvanderveldt
Copy link

simonvanderveldt commented Feb 22, 2022

@villebro We're using Spark as the SQL backend, I assumed all of this was happening in shared code but since you're so explicitly mentioning Postgres it sounds like that's not the case?

[edit] @cabo40 is right, your example uses data that isn't affected by this. Try 9999-12-31 instead.

@cabo40
Copy link
Contributor Author

cabo40 commented Feb 23, 2022

I think it will run since no casting needs to be done with the query you used. For me it breaks with SELECT TIMESTAMP '01-01-1000';.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[sql-lab]Pyarrow to pandas fails when timestamp is out of bounds
5 participants