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

[Bug fix] Divide by 1000.000 in epoch_ms_to_dttm() to not lose precision in Presto #5211

Merged
merged 18 commits into from Jul 4, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jun 15, 2018

Context:

presto> SELECT CAST(from_unixtime((1528761599980/1000.0)) AS TIMESTAMP);
-------------------------
 2018-06-12 00:00:00.000

presto> SELECT CAST(from_unixtime((1528761599980/1000)) AS TIMESTAMP);
-------------------------
 2018-06-11 23:59:59.000

presto> SELECT CAST(from_unixtime((1528761599980/1000.00)) AS TIMESTAMP);
-------------------------
 2018-06-11 23:59:59.980

In an unfortunate event where the entry is < 1 second from the next day, the generated query casts it to the next day. This shouldn't be a problem in MySQL, but it is in Presto.
screen shot 2018-06-14 at 1 54 36 pm

@ghost ghost changed the title Evelynturner/prestofix [Bug fix] Divide by 1000.000 in epoch_ms_to_dttm() to not lose precision in Presto Jun 15, 2018
@john-bodley
Copy link
Member

@evelynturner I'm not sure this is necessary given that dividing an integer by a float returns a float, i.e.,

presto:default> SELECT CAST(from_unixtime((1528761599980/1000.0)) AS TIMESTAMP);
 2018-06-11 23:59:59.980 

works as expected.

@ghost
Copy link
Author

ghost commented Jun 18, 2018

@john-bodley That's because of your parse_decimal_literals_as_double setting..

For compatibility reasons decimal literals without explicit type specifier (e.g. 1.2) are treated as the values of the DOUBLE type by default, but this is subject to change in future releases. This behavior is controlled by:
System wide property: parse-decimal-literals-as-double
Session wide property: parse_decimal_literals_as_double

https://prestodb.io/docs/current/language/types.html#floating-point

I can always set it as true, but I didn't see any harm in accommodating to the other setting as well. Maybe we can document it somewhere that people need to set those properties as true for Presto then.

@mistercrunch
Copy link
Member

LGTM though can you rebase?

@codecov-io
Copy link

Codecov Report

Merging #5211 into master will decrease coverage by 16.15%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #5211       +/-   ##
===========================================
- Coverage   77.48%   61.32%   -16.16%     
===========================================
  Files          44      369      +325     
  Lines        8749    23488    +14739     
  Branches        0     2717     +2717     
===========================================
+ Hits         6779    14405     +7626     
- Misses       1970     9071     +7101     
- Partials        0       12       +12
Impacted Files Coverage Δ
superset/db_engine_specs.py 54.28% <ø> (+0.56%) ⬆️
superset/sql_lab.py 71.6% <0%> (-3.81%) ⬇️
superset/dataframe.py 94.49% <0%> (-3.21%) ⬇️
superset/views/core.py 72.96% <0%> (-1.71%) ⬇️
superset/models/core.py 85.29% <0%> (-1.26%) ⬇️
superset/__init__.py 72.32% <0%> (-0.79%) ⬇️
superset/utils.py 87.8% <0%> (-0.72%) ⬇️
superset/exceptions.py 100% <0%> (ø) ⬆️
superset/connectors/druid/models.py 80.56% <0%> (ø) ⬆️
...sets/src/explore/components/DisplayQueryButton.jsx 84.21% <0%> (ø)
... and 328 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 de0aaf4...c6abfe9. Read the comment docs.

@mistercrunch mistercrunch merged commit ad9103f into apache:master Jul 4, 2018
timifasubaa pushed a commit to airbnb/superset-fork that referenced this pull request Jul 25, 2018
…ion in Presto (apache#5211)

* Fix how the annotation layer interpretes the timestamp string without timezone info; use it as UTC

* [Bug fix] Fixed/Refactored annotation layer code so that non-timeseries annotations are applied based on the updated chart object after adding all data

* [Bug fix] Fixed/Refactored annotation layer code so that non-timeseries annotations are applied based on the updated chart object after adding all data

* Fixed indentation

* Fix the key string value in case series.key is a string

* Fix the key string value in case series.key is a string

* [Bug fix] Divide by 1000.000 in epoch_ms_to_dttm() to not lose precision in Presto

* [Bug fix] Divide by 1000.000 in epoch_ms_to_dttm() to not lose precision in Presto
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
…ion in Presto (apache#5211)

* Fix how the annotation layer interpretes the timestamp string without timezone info; use it as UTC

* [Bug fix] Fixed/Refactored annotation layer code so that non-timeseries annotations are applied based on the updated chart object after adding all data

* [Bug fix] Fixed/Refactored annotation layer code so that non-timeseries annotations are applied based on the updated chart object after adding all data

* Fixed indentation

* Fix the key string value in case series.key is a string

* Fix the key string value in case series.key is a string

* [Bug fix] Divide by 1000.000 in epoch_ms_to_dttm() to not lose precision in Presto

* [Bug fix] Divide by 1000.000 in epoch_ms_to_dttm() to not lose precision in Presto
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024
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 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants