fix: JSON serializers #22029

john-bodley · 2022-11-03T19:09:49Z

SUMMARY

I'm not exactly sure why this issue didn't surface prior to #21936 when np.vectorize was being leveraged, but the JSON serialization failed when an np.array was undefined, i.e., np.array(None), i.e., in SQL Lab one would see the following,

Unserializable object None of type <class 'numpy.ndarray'>

if the column was an array but the array was null.

The issue was that the base_json_conv treated a return value of None as representing an object which wasn't able to be converted however some converted types can have a return value of None, i.e., np.array(None).tolist() is None.

The fix is to make base_json_conv raise a TypeError if the type is unknown and to reorganize the logic for the JSON serializers. See the inline comments for more details.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Added unit tests.

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

john-bodley · 2022-11-03T19:11:28Z

superset/utils/core.py

@@ -581,47 +588,60 @@ def base_json_conv(  # pylint: disable=inconsistent-return-statements
        except Exception:  # pylint: disable=broad-except
            return "[bytes]"

+    raise TypeError(f"Unserializable object {obj} of type {type(obj)}")


Previously this would return None however there was no way to distinguish between the conversion of np.array(None) which returns None and an unserializable object.

john-bodley · 2022-11-03T19:12:12Z

superset/utils/core.py


-def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> str:
+def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> Any:


The return type was wrong. Per the base_json_conv method there's a slew of viable return types.

john-bodley · 2022-11-03T19:12:22Z

superset/utils/core.py



-def pessimistic_json_iso_dttm_ser(obj: Any) -> str:
+def pessimistic_json_iso_dttm_ser(obj: Any) -> Any:


The return type was wrong. Per the base_json_conv method there's a slew of viable return types.

john-bodley · 2022-11-03T19:14:56Z

superset/utils/core.py

-    else:
+        return obj.isoformat()
+
+    try:


The logic has been flipped. Rather than trying the base conversion—which now throws—first and then the more specific temporal types, the logic now tries to convert the more specific and then falls back to the more generic. This ensures there's less error handling.

john-bodley · 2022-11-03T19:15:03Z

superset/utils/core.py

-    else:
-        raise TypeError("Unserializable object {} of type {}".format(obj, type(obj)))
-    return obj
+        return datetime_to_epoch(obj)


john-bodley · 2022-11-03T19:15:22Z

tests/integration_tests/utils_tests.py

-        assert isinstance(base_json_conv(uuid.uuid4()), str) is True
-        assert isinstance(base_json_conv(time()), str) is True
-        assert isinstance(base_json_conv(timedelta(0)), str) is True
+        assert isinstance(base_json_conv(np.bool_(1)), bool)


The is True is unnecessary.

john-bodley · 2022-11-03T19:15:34Z

tests/integration_tests/utils_tests.py

+        assert isinstance(base_json_conv(np.bool_(1)), bool)
+        assert isinstance(base_json_conv(np.int64(1)), int)
+        assert isinstance(base_json_conv(np.array([1, 2, 3])), list)
+        assert base_json_conv(np.array(None)) is None


This is a new test.

john-bodley · 2022-11-03T19:15:43Z

tests/integration_tests/utils_tests.py

+        assert isinstance(base_json_conv(bytes()), str)
+        assert base_json_conv(bytes("", encoding="utf-16")) == ["bytes"]
+
+        with pytest.raises(TypeError):


This is a new test.

john-bodley · 2022-11-03T19:16:18Z

tests/integration_tests/utils_tests.py

@@ -105,19 +111,26 @@ def test_json_iso_dttm_ser(self):
        assert json_iso_dttm_ser(dttm) == dttm.isoformat()
        assert json_iso_dttm_ser(dt) == dt.isoformat()
        assert json_iso_dttm_ser(t) == t.isoformat()
+        assert json_int_dttm_ser(np.int64(1)) == 1


The non-temporal case was never tested.

john-bodley · 2022-11-03T19:16:23Z

tests/integration_tests/utils_tests.py

@@ -94,9 +94,15 @@ def test_json_int_dttm_ser(self):
        assert json_int_dttm_ser(datetime(1970, 1, 1)) == 0
        assert json_int_dttm_ser(date(1970, 1, 1)) == 0
        assert json_int_dttm_ser(dttm + timedelta(milliseconds=1)) == (ts + 1)
+        assert json_int_dttm_ser(np.int64(1)) == 1


The non-temporal case was never tested.

ktmud · 2022-11-03T20:11:37Z

superset/utils/core.py

+        return datetime_to_epoch(obj)
+
+    if isinstance(obj, date):
+        return (obj - EPOCH.date()).total_seconds() * 1000


i think we can use obj.timestamp() now since we are on python 3.3+.

@ktmud this is a date object and not a datetime object.

codecov · 2022-11-04T00:02:38Z

Codecov Report

Merging #22029 (be35ab9) into master (d3f930a) will decrease coverage by 11.44%.
The diff coverage is 63.93%.

❗ Current head be35ab9 differs from pull request most recent head 28cccf8. Consider uploading reports for the commit 28cccf8 to get more accurate results

@@             Coverage Diff             @@
##           master   #22029       +/-   ##
===========================================
- Coverage   66.96%   55.51%   -11.45%     
===========================================
  Files        1807     1813        +6     
  Lines       69222    69426      +204     
  Branches     7403     7448       +45     
===========================================
- Hits        46352    38541     -7811     
- Misses      20965    28965     +8000     
- Partials     1905     1920       +15

Flag	Coverage Δ
hive	`?`
mysql	`?`
postgres	`?`
presto	`52.74% <39.90%> (-0.08%)`	⬇️
python	`57.66% <62.01%> (-23.83%)`	⬇️
sqlite	`?`
unit	`51.18% <58.17%> (+0.09%)`	⬆️

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

Impacted Files	Coverage Δ
...ackages/superset-ui-chart-controls/src/fixtures.ts	`100.00% <ø> (ø)`
...d/packages/superset-ui-chart-controls/src/index.ts	`100.00% <ø> (ø)`
...perset-ui-chart-controls/src/sections/sections.tsx	`71.42% <0.00%> (-16.08%)`	⬇️
...chart-controls/src/shared-controls/dndControls.tsx	`58.33% <ø> (ø)`
...d/packages/superset-ui-chart-controls/src/types.ts	`100.00% <ø> (ø)`
...kages/superset-ui-core/src/query/types/Operator.ts	`100.00% <ø> (ø)`
...packages/superset-ui-core/src/query/types/Query.ts	`100.00% <ø> (ø)`
...ackages/superset-ui-core/src/utils/featureFlags.ts	`100.00% <ø> (ø)`
.../legacy-plugin-chart-histogram/src/controlPanel.ts	`40.00% <0.00%> (ø)`
...lugin-chart-handlebars/src/plugin/controlPanel.tsx	`50.00% <ø> (ø)`
... and 393 more

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

(cherry picked from commit 6bbf4f8)

villebro · 2022-11-04T06:52:05Z

tests/integration_tests/utils_tests.py

-        assert isinstance(base_json_conv(uuid.uuid4()), str) is True
-        assert isinstance(base_json_conv(time()), str) is True
-        assert isinstance(base_json_conv(timedelta(0)), str) is True
+        assert isinstance(base_json_conv(np.bool_(1)), bool)


Nit: when adding stuff to old tests like this I nowadays refactor them by 1) converting them to functional pytest tests 2) to use pytest.mark.parameterize to DRY the repeated logic.

pull-request-size bot added the size/L label Nov 3, 2022

john-bodley commented Nov 3, 2022

View reviewed changes

john-bodley marked this pull request as ready for review November 3, 2022 19:23

john-bodley requested review from villebro and ktmud November 3, 2022 19:23

ktmud approved these changes Nov 3, 2022

View reviewed changes

john-bodley force-pushed the john-bodley--fix-base_json_conv branch 2 times, most recently from 6acb9e1 to 3516a32 Compare November 3, 2022 22:37

fix: JSON serializers

28cccf8

john-bodley force-pushed the john-bodley--fix-base_json_conv branch from 3516a32 to 28cccf8 Compare November 3, 2022 23:55

john-bodley merged commit 6bbf4f8 into apache:master Nov 4, 2022

john-bodley added a commit to airbnb/superset-fork that referenced this pull request Nov 4, 2022

fix: JSON serializers (apache#22029)

245a5f7

(cherry picked from commit 6bbf4f8)

john-bodley added a commit to airbnb/superset-fork that referenced this pull request Nov 4, 2022

fix: JSON serializers (apache#22029)

dedb3a6

(cherry picked from commit 6bbf4f8)

villebro reviewed Nov 4, 2022

View reviewed changes

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: JSON serializers #22029

fix: JSON serializers #22029

john-bodley commented Nov 3, 2022 •

edited

Loading

john-bodley Nov 3, 2022

john-bodley Nov 3, 2022

john-bodley Nov 3, 2022

john-bodley Nov 3, 2022

john-bodley Nov 3, 2022

john-bodley Nov 3, 2022

john-bodley Nov 3, 2022

john-bodley Nov 3, 2022

john-bodley Nov 3, 2022

john-bodley Nov 3, 2022

ktmud Nov 3, 2022

john-bodley Nov 3, 2022

codecov bot commented Nov 4, 2022

villebro Nov 4, 2022


		def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> str:
		def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> Any:



		def pessimistic_json_iso_dttm_ser(obj: Any) -> str:
		def pessimistic_json_iso_dttm_ser(obj: Any) -> Any:

fix: JSON serializers #22029

fix: JSON serializers #22029

Conversation

john-bodley commented Nov 3, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 4, 2022

Codecov Report

Choose a reason for hiding this comment

john-bodley commented Nov 3, 2022 •

edited

Loading