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: invalid float number format by json-bigint #21968

Conversation

justinpark
Copy link
Member

@justinpark justinpark commented Oct 28, 2022

SUMMARY

This commit fixes the regression from #21937
There's a known issue for floating value formatting on current latest version of json-bigint open source.

Since the main contributor no longer maintain the new release, this commit replaces the open-source library by json-bigint-native which is branched out from json-bigint@1.0.0 (latest version) and fixes the known bugs including float value issue.
This commit also adds the extra specs to verify the floating value formatting.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before: (returns NaN values)

saft-p-90-guest-active-topline-trend-2022-10-28T19-10-08_622Z

After:

saft-p-90-guest-active-topline-trend-2022-10-28T19-10-14_909Z

TESTING INSTRUCTIONS

Create a chart with floating numbers

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • 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

@justinpark
Copy link
Member Author

cc: @zhaoyongjie

@codecov
Copy link

codecov bot commented Oct 28, 2022

Codecov Report

Merging #21968 (1bdbc21) into master (7f563cf) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #21968      +/-   ##
==========================================
- Coverage   66.96%   66.95%   -0.01%     
==========================================
  Files        1807     1807              
  Lines       69221    69241      +20     
  Branches     7402     7407       +5     
==========================================
+ Hits        46352    46363      +11     
- Misses      20964    20969       +5     
- Partials     1905     1909       +4     
Flag Coverage Δ
javascript 53.40% <ø> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...et-ui-core/src/connection/callApi/parseResponse.ts 100.00% <ø> (ø)
...-frontend/src/components/FilterableTable/index.tsx 72.67% <ø> (ø)
...ters/FiltersConfigModal/FiltersConfigForm/utils.ts 77.77% <0.00%> (-4.58%) ⬇️
...d/src/SqlLab/components/SaveDatasetModal/index.tsx 52.27% <0.00%> (-0.61%) ⬇️
superset-frontend/src/views/App.tsx 0.00% <0.00%> (ø)
...Filters/FiltersConfigModal/FilterConfigurePane.tsx 100.00% <0.00%> (ø)
...ilters/FilterBar/FilterConfigurationLink/index.tsx 100.00% <0.00%> (ø)
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx 59.50% <0.00%> (ø)
superset-frontend/src/logger/LogUtils.ts 96.15% <0.00%> (+0.15%) ⬆️
...eFilters/FiltersConfigModal/FiltersConfigModal.tsx 63.63% <0.00%> (+0.59%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ktmud
Copy link
Member

ktmud commented Oct 28, 2022

Can you add a link to the forked repo and npm package?

@justinpark
Copy link
Member Author

Can you add a link to the forked repo and npm package?

npm package: https://www.npmjs.com/package/json-bigint-native
repo: https://github.com/vogievetsky/json-bigint

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a remark/question

@@ -145,6 +145,7 @@
"js-levenshtein": "^1.1.6",
"js-yaml-loader": "^1.2.2",
"json-bigint": "^1.0.0",
"json-bigint-native": "^1.2.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If json-bigint is no longer maintained, can we remove that dep (on line 147)?

Copy link
Member

@zhaoyongjie zhaoyongjie Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the json-bigint-native says that added TypeScript types, the line 43 also need to be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will follow up with this

@zhaoyongjie zhaoyongjie self-requested a review October 31, 2022 04:01
@john-bodley john-bodley merged commit 3bb9187 into apache:master Oct 31, 2022
john-bodley pushed a commit to airbnb/superset-fork that referenced this pull request Oct 31, 2022
justinpark added a commit to justinpark/superset that referenced this pull request Nov 1, 2022
john-bodley pushed a commit to airbnb/superset-fork that referenced this pull request Nov 2, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 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 size/S 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants