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 #371 for number larger than 2.14 billion #374

Merged
merged 3 commits into from
May 15, 2019

Conversation

vin-e
Copy link

@vin-e vin-e commented May 13, 2019

Fixes issue (#371) with large numbers by removing the bitwise operation used on row index. I did not change bitwise operations done against column indices.

@6pac
Copy link
Owner

6pac commented May 14, 2019

Sorry, but I have to ask the obvious ... you have more than 2 billion rows?

Also, if you're going to go into the floating point numeric scope, you're going to have problems with precision: https://stackoverflow.com/questions/33773296/is-there-or-isnt-there-an-integer-type-in-javascript

edit: Ah, OK bitwise is 32 bit, we're aiming for the full SAFE_INT scope. Still checking my javascript apocrypha to see what the impact is...

x                 x | 0      +x	
----              -----      ---
null                0         0
undefined           0         NaN
NaN                 0         NaN

So I'm not sure exactly what empty value this is working against, but I think it would be more likely to be an undefined than a null. Give the above, the following might be safer:

return (row ? row :  0);

@vin-e
Copy link
Author

vin-e commented May 14, 2019

Thanks for the feedback and I will update the code as recommended.

Note, I took the path or using a + because there is a comment on line 3218 that reads: // This is a string, so it needs to be cast back to a number. My assumption was that we were validating the number.

To answer your other question... Yes, we have very large datasets with some reaching into the multiple billions. An internal user identified the bug when attempting to get the context menu on a row in the 5 billion range.

@vin-e
Copy link
Author

vin-e commented May 14, 2019

FYI, The row is a string so we do need to parse the integer. Which is addressed on my latest commit.

@6pac
Copy link
Owner

6pac commented May 15, 2019

Awesome. Just trying to keep the code as bulletproof as possible!

@6pac 6pac merged commit 4576936 into 6pac:master May 15, 2019
@vin-e vin-e deleted the bugfix/large-number branch May 15, 2019 05:15
@6pac 6pac mentioned this pull request Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants