refactor: Unify all json.(loads|dumps) usage to utils.json #28702
Conversation
github-actions
bot
added
risk:db-migration
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 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
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. |
eyalezer
force-pushed
the
json_utils
branch
2 times, most recently
from
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]>
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: