[BUGFIX]: JavaScripts max int is 2^53 - 1, longs are bigger #4005

fabianmenges · 2017-12-05T02:08:00Z

Twitter uses very long IDs for entities. This causes issues because Java Script can not handle these long IDs and will round them by treating them as a float.

This is a very hack fix, that will just convert every integer that is longer than the longest integer supported by JS to a string for both SQL Lab as well as the default table visualization.
This works since both python and JS are dynamically typed, but its not great.

Any suggestions how we should handle this correctly?

rumbin · 2017-12-05T06:14:29Z

I think #3188 is related...

bolkedebruin · 2017-12-05T07:46:32Z

I think using the BigInt package https://www.npmjs.com/package/big-integer might be smarter. Also DRY being applied would be nice and a test.

xrmx

Hopefully BigInt is coming in JS:
https://github.com/tc39/proposal-bigint

xrmx · 2017-12-05T08:29:31Z

superset/viz.py

@@ -935,7 +946,10 @@ def to_series(self, df, classed='', title_suffix=''):
            ys = series[name]
            if df[name].dtype.kind not in 'biufc':
                continue
-            series_title = name
+            if isinstance(name, (list, tuple)):


This hunk looks unrelated

This is actually related. When you group by a numeric entity you can run into the same problem that the ID is to large to be displayed correctly in the legend or pop over. However we can just treat everything in the title a string since we can be sure its not used for anything mathematical.

xrmx · 2017-12-05T08:31:45Z

superset/dataframe.py

+            for k, v in list(d.items()):
+                # if an int is too big for Java Script to handle
+                # convert it to a string
+                if isinstance(v, int):


What about moving the constant and the loop in an helper?

Sure, will do.

Good idea, def handle_js_int_overflow(data) or something

fabianmenges · 2017-12-05T15:46:00Z

@bolkedebruin @xrmx I looked at the various BigInt packages in JS. The problem is that I don't think d3 will handle BigInts correctly, which is why I'm converting large numbers to string only in cases where I'm pretty sure we are not going to do any computations with them, specifically:

The legend of line charts
Any table visualization (SQL lab or table slice)

If a value is supposed to be displayed/ingested in a visualization (e.g. line char, pie chart, ...) it is usually fine for it to loose precision since you usually don't care about the exact number anymore.
Unless JS supports a real bigint and all of our visualization libraries support it we will probably have to convert big integers to string on a visualization by visualization level.

An alternative to what I did in this PR would be to define that all integers for the table visualizations are always converted to a string (regardless of their size) and then fix up the sorting logic for sorting on the client side.

mistercrunch

This is a tough one to work around of... while your fix seems like it would work for dimensions, what happens with overflowing metrics?

mistercrunch · 2017-12-05T18:03:52Z

superset/viz.py

@@ -935,7 +946,10 @@ def to_series(self, df, classed='', title_suffix=''):
            ys = series[name]
            if df[name].dtype.kind not in 'biufc':
                continue
-            series_title = name
+            if isinstance(name, (list, tuple)):
+                series_title = [str(title) for title in name]


does that affect bar chart sorting?

It should not since we are treating the title as string in the JS anyways.

fabianmenges · 2017-12-05T19:34:25Z

@mistercrunch A couple screenshots how everything behaves:
SQL Lab:
You can see that the bigint is rendered correctly when its a dimension, if you put a sum around it, it would still work

Table Viz
Grouped by time and summed. The result shows a loss of precision:

Non Grouped the precision is maintained

Bar Charts

mistercrunch · 2018-01-04T17:22:35Z

superset/viz.py

            records=df.to_dict(orient='records'),
            columns=list(df.columns),
        )

+        for d in data.get('records', dict()):


let's also put this into a BaseViz.handle_js_int_overflow method

jeffreythewang · 2018-03-13T19:28:41Z

Any updates on this besides a rebase (and a possible test fix)?

…ently used as IDs this is a hacky fix.

codecov-io · 2018-03-19T14:40:52Z

Codecov Report

Merging #4005 into master will increase coverage by 0.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #4005      +/-   ##
==========================================
+ Coverage   71.22%   71.23%   +0.01%     
==========================================
  Files         190      190              
  Lines       14880    14900      +20     
  Branches     1098     1098              
==========================================
+ Hits        10598    10614      +16     
- Misses       4279     4283       +4     
  Partials        3        3

Impacted Files	Coverage Δ
superset/utils.py	`87.85% <100%> (+0.02%)`	⬆️
superset/viz.py	`78.43% <80%> (-0.03%)`	⬇️
superset/dataframe.py	`96.55% <87.5%> (-0.95%)`	⬇️

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 97afcd5...f43ad4f. Read the comment docs.

) * [BUGFIX]: Java scripts max int is 2^53 - 1 longs are bigger and frequently used as IDs this is a hacky fix. * Keep tuple as tuple

fabianmenges force-pushed the fmenges/hacky_js_long_fix branch 2 times, most recently from 81ab7e7 to b5a1f67 Compare December 5, 2017 02:51

xrmx reviewed Dec 5, 2017

View reviewed changes

mistercrunch reviewed Dec 5, 2017

View reviewed changes

fabianmenges force-pushed the fmenges/hacky_js_long_fix branch 2 times, most recently from 09a72c2 to 31c03d6 Compare December 6, 2017 15:56

fabianmenges mentioned this pull request Jan 4, 2018

[P00 - sql lab] data results view truncates integers at 16 digits #4107

Closed

3 tasks

mistercrunch reviewed Jan 4, 2018

View reviewed changes

fabianmenges changed the title ~~[BUGFIX]: Java scripts max int is 2^53 - 1, longs are bigger~~ [BUGFIX]: JavaScripts max int is 2^53 - 1, longs are bigger Jan 9, 2018

fabianmenges force-pushed the fmenges/hacky_js_long_fix branch 2 times, most recently from 6f9cce5 to 2ee6990 Compare January 16, 2018 14:41

jeffreythewang force-pushed the fmenges/hacky_js_long_fix branch from 2ee6990 to 1c89942 Compare March 16, 2018 17:26

Fabian and others added 2 commits March 19, 2018 10:19

[BUGFIX]: Java scripts max int is 2^53 - 1 longs are bigger and frequ…

75cb5b8

…ently used as IDs this is a hacky fix.

Keep tuple as tuple

f43ad4f

jeffreythewang force-pushed the fmenges/hacky_js_long_fix branch from 1c89942 to f43ad4f Compare March 19, 2018 14:19

mistercrunch merged commit 9a79d33 into apache:master Apr 4, 2018

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX]: JavaScripts max int is 2^53 - 1, longs are bigger #4005

[BUGFIX]: JavaScripts max int is 2^53 - 1, longs are bigger #4005

fabianmenges commented Dec 5, 2017

rumbin commented Dec 5, 2017

bolkedebruin commented Dec 5, 2017

xrmx left a comment

xrmx Dec 5, 2017

fabianmenges Dec 5, 2017

xrmx Dec 5, 2017

fabianmenges Dec 5, 2017

mistercrunch Jan 4, 2018

fabianmenges commented Dec 5, 2017

mistercrunch left a comment

mistercrunch Dec 5, 2017

fabianmenges Dec 5, 2017

fabianmenges commented Dec 5, 2017 •

edited

Loading

mistercrunch Jan 4, 2018

jeffreythewang commented Mar 13, 2018 •

edited

Loading

codecov-io commented Mar 19, 2018

[BUGFIX]: JavaScripts max int is 2^53 - 1, longs are bigger #4005

[BUGFIX]: JavaScripts max int is 2^53 - 1, longs are bigger #4005

Conversation

fabianmenges commented Dec 5, 2017

rumbin commented Dec 5, 2017

bolkedebruin commented Dec 5, 2017

xrmx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianmenges commented Dec 5, 2017

mistercrunch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianmenges commented Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

jeffreythewang commented Mar 13, 2018 • edited Loading

codecov-io commented Mar 19, 2018

Codecov Report

fabianmenges commented Dec 5, 2017 •

edited

Loading

jeffreythewang commented Mar 13, 2018 •

edited

Loading