-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
refactor: Unify all json.(loads|dumps) usage to utils.json #28702
Conversation
json_utils.dumps( | ||
payload, default=json_utils.json_iso_dttm_ser, ignore_nan=True | ||
), | ||
json.dumps(payload, default=json.json_iso_dttm_ser, ignore_nan=True), |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Stack trace information
Stack trace information
Stack trace information
Stack trace information
Stack trace information
Stack trace information
Stack trace information
Stack trace information
Stack trace information
Stack trace information
Stack trace information
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #28702 +/- ##
===========================================
+ Coverage 60.48% 83.47% +22.98%
===========================================
Files 1931 523 -1408
Lines 76236 37575 -38661
Branches 8568 0 -8568
===========================================
- Hits 46114 31365 -14749
+ Misses 28017 6210 -21807
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
85e6fab
to
f697f25
Compare
@mistercrunch - here's the second part of the refactor, as expected this is a huge PR: 232 files changed |
OMG glad to see this, happy to help fast-merge this since it'll conflict with everything else otherwise. Oh as a follow up, or maybe something we may want to bundle here -> @betodealmeida mentioned that there's a fairly easy way for us to add a linting rule that prevents people from doing simple |
i'll rebase it quickly so it won't catch up more conflicts... regarding the linter, it's a great idea and it looks like it should be plausible by adding a custom mypy plugin for example... but there's more research needed to be done here. |
@betodealmeida - awesome, so it's even easier than i thought... i'll look into it now and test it |
@mistercrunch - Rebased before it's too late
|
Amazing, this is a massive refactor that should make everything json-related much more manageable. Interestingly python's standard lib json IS simplejson (see here https://stackoverflow.com/questions/712791/what-are-the-differences-between-json-and-simplejson-python-modules), but simplejson is typically ahead. Also having json all in one place allows us to consider things like https://pypi.org/project/ujson/ and do things like what triggered this refactor (improve utf-8 support + error handling) centrally. |
It's interesting that you brought it up. After I finished refactoring all the json.(loads|dumps) to utilize the json module, one of the first things I did was to check if ujson actually provides any significant performance enhancements to superset. it seems to be functioning fine, but I must admit that I haven't thoroughly tested it to accurately measure the extent of its performance improvements. Nevertheless, if anyone is interested in giving it a try, I still have the branch available here: https://github.com/eyalezer/superset/tree/ujson. |
Hey @eyalezer this is great. If you haven't, feel free to join the Apache Superset Slack. I'd be happy to help if you wish to contribute more to the project! |
) Co-authored-by: Eyal Ezer <[email protected]>
) Co-authored-by: Eyal Ezer <[email protected]>
SUMMARY
Second phase of the json migration to use the new utils.json module
After completing the initial phase of creating the utils.json module as mentioned in the following link: #28522, we are now moving on to the second phase. This phase involves consolidating all json usage and transitioning to the utilization of the newly created module.
During this phase: