-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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: BIGINT rendering regression in chartAction #21937
fix: BIGINT rendering regression in chartAction #21937
Conversation
}; | ||
|
||
return SupersetClient.post(querySettings); | ||
return SupersetClient.post(querySettings).then(({ text, response }) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justinpark can we add a simply unit test which includes an integer with a value greater than 2^53 -1?
How about we change the default |
@ktmud good idea. I originally thought same way. let me add new parseMethod type |
187cfde
to
8dbea45
Compare
'json-bigint', | ||
); | ||
expect(`${responseBigNumber.json.value}`).toEqual('9223372036854775807'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the test!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also mock a -2^63
to test int64? and we also need to add a comment to point out the integer type is that singed int64.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhaoyongjie good point. I added the spec for negative number.
Codecov Report
@@ Coverage Diff @@
## master #21937 +/- ##
==========================================
+ Coverage 66.90% 66.92% +0.01%
==========================================
Files 1807 1807
Lines 69183 69182 -1
Branches 7405 7403 -2
==========================================
+ Hits 46288 46298 +10
+ Misses 20985 20973 -12
- Partials 1910 1911 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leave an unblocking suggestion. LGTM
'json-bigint', | ||
); | ||
expect(`${responseBigNumber.json.value}`).toEqual('9223372036854775807'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also mock a -2^63
to test int64? and we also need to add a comment to point out the integer type is that singed int64.
(cherry picked from commit 4002406)
(cherry picked from commit 4002406)
SUMMARY
Since chart data api
/api/v1/chart/data
responds the bigint in number format, the number beyond the JS limit that are replaced with the special number.This commit introduces new parseMethod
json-bigint
to fix the unmatched value displaying for these big numbers.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
SELECT 568485382077428896 as BIGNUM
ADDITIONAL INFORMATION
cc: @john-bodley @ktmud