You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/11/03 19:09:53 UTC
[GitHub] [superset] john-bodley opened a new pull request, #22029: fix: JSON serializers
john-bodley opened a new pull request, #22029:
URL: https://github.com/apache/superset/pull/22029
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #22029: fix: JSON serializers
Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #22029:
URL: https://github.com/apache/superset/pull/22029#discussion_r1013430507
##########
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)}")
+
-def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> str:
+def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> Any:
"""
- json serializer that deals with dates
+ A JSON serializer that deals with dates by serializing them to ISO 8601.
- >>> dttm = datetime(1970, 1, 1)
- >>> json.dumps({'dttm': dttm}, default=json_iso_dttm_ser)
- '{"dttm": "1970-01-01T00:00:00"}'
+ >>> json.dumps({'dttm': datetime(1970, 1, 1)}, default=json_iso_dttm_ser)
+ '{"dttm": "1970-01-01T00:00:00"}'
+
+ :param obj: The serializable object
+ :param pessimistic: Whether to be pessimistic regarding serialization
+ :returns: The JSON compatible form
+ :raises TypeError: If the non-pessimistic object cannot be serialized
"""
- val = base_json_conv(obj)
- if val is not None:
- return val
+
if isinstance(obj, (datetime, date, pd.Timestamp)):
- obj = obj.isoformat()
- else:
+ return obj.isoformat()
+
+ try:
+ return base_json_conv(obj)
+ except TypeError as ex:
if pessimistic:
- return "Unserializable [{}]".format(type(obj))
+ return f"Unserializable [{type(obj)}]"
- raise TypeError("Unserializable object {} of type {}".format(obj, type(obj)))
- return obj
+ raise ex
-def pessimistic_json_iso_dttm_ser(obj: Any) -> str:
+def pessimistic_json_iso_dttm_ser(obj: Any) -> Any:
"""Proxy to call json_iso_dttm_ser in a pessimistic way
If one of object is not serializable to json, it will still succeed"""
return json_iso_dttm_ser(obj, pessimistic=True)
-def json_int_dttm_ser(obj: Any) -> float:
- """json serializer that deals with dates"""
- val = base_json_conv(obj)
- if val is not None:
- return val
+def json_int_dttm_ser(obj: Any) -> Any:
+ """
+ A JSON serializer that deals with dates by serializing them to EPOCH.
+
+ >>> json.dumps({'dttm': datetime(1970, 1, 1)}, default=json_int_dttm_ser)
+ '{"dttm": 0}'
+
+ :param obj: The serializable object
+ :returns: The JSON compatible form
+ :raises TypeError: If the object cannot be serialized
+ """
+
if isinstance(obj, (datetime, pd.Timestamp)):
- obj = datetime_to_epoch(obj)
- elif isinstance(obj, date):
- obj = (obj - EPOCH.date()).total_seconds() * 1000
- else:
- raise TypeError("Unserializable object {} of type {}".format(obj, type(obj)))
- return obj
+ return datetime_to_epoch(obj)
+
+ if isinstance(obj, date):
+ return (obj - EPOCH.date()).total_seconds() * 1000
Review Comment:
@ktmud this is a `date` object and not a `datetime` object.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley merged pull request #22029: fix: JSON serializers
Posted by GitBox <gi...@apache.org>.
john-bodley merged PR #22029:
URL: https://github.com/apache/superset/pull/22029
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #22029: fix: JSON serializers
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22029:
URL: https://github.com/apache/superset/pull/22029#issuecomment-1302806844
# [Codecov](https://codecov.io/gh/apache/superset/pull/22029?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22029](https://codecov.io/gh/apache/superset/pull/22029?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (be35ab9) into [master](https://codecov.io/gh/apache/superset/commit/d3f930a5575f03bc5f4f468b8bf37e834bf2aa4d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d3f930a) will **decrease** coverage by `11.44%`.
> The diff coverage is `63.93%`.
> :exclamation: Current head be35ab9 differs from pull request most recent head 28cccf8. Consider uploading reports for the commit 28cccf8 to get more accurate results
```diff
@@ 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%)` | :arrow_down: |
| python | `57.66% <62.01%> (-23.83%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `51.18% <58.17%> (+0.09%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22029?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ackages/superset-ui-chart-controls/src/fixtures.ts](https://codecov.io/gh/apache/superset/pull/22029/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL2ZpeHR1cmVzLnRz) | `100.00% <ø> (ø)` | |
| [...d/packages/superset-ui-chart-controls/src/index.ts](https://codecov.io/gh/apache/superset/pull/22029/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL2luZGV4LnRz) | `100.00% <ø> (ø)` | |
| [...perset-ui-chart-controls/src/sections/sections.tsx](https://codecov.io/gh/apache/superset/pull/22029/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NlY3Rpb25zL3NlY3Rpb25zLnRzeA==) | `71.42% <0.00%> (-16.08%)` | :arrow_down: |
| [...chart-controls/src/shared-controls/dndControls.tsx](https://codecov.io/gh/apache/superset/pull/22029/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9kbmRDb250cm9scy50c3g=) | `58.33% <ø> (ø)` | |
| [...d/packages/superset-ui-chart-controls/src/types.ts](https://codecov.io/gh/apache/superset/pull/22029/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3R5cGVzLnRz) | `100.00% <ø> (ø)` | |
| [...kages/superset-ui-core/src/query/types/Operator.ts](https://codecov.io/gh/apache/superset/pull/22029/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvT3BlcmF0b3IudHM=) | `100.00% <ø> (ø)` | |
| [...packages/superset-ui-core/src/query/types/Query.ts](https://codecov.io/gh/apache/superset/pull/22029/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvUXVlcnkudHM=) | `100.00% <ø> (ø)` | |
| [...ackages/superset-ui-core/src/utils/featureFlags.ts](https://codecov.io/gh/apache/superset/pull/22029/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdXRpbHMvZmVhdHVyZUZsYWdzLnRz) | `100.00% <ø> (ø)` | |
| [.../legacy-plugin-chart-histogram/src/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/22029/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWhpc3RvZ3JhbS9zcmMvY29udHJvbFBhbmVsLnRz) | `40.00% <0.00%> (ø)` | |
| [...lugin-chart-handlebars/src/plugin/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/22029/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtaGFuZGxlYmFycy9zcmMvcGx1Z2luL2NvbnRyb2xQYW5lbC50c3g=) | `50.00% <ø> (ø)` | |
| ... and [393 more](https://codecov.io/gh/apache/superset/pull/22029/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #22029: fix: JSON serializers
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #22029:
URL: https://github.com/apache/superset/pull/22029#discussion_r1013676609
##########
tests/integration_tests/utils_tests.py:
##########
@@ -105,19 +106,31 @@ 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_iso_dttm_ser(np.int64(1)) == 1
+
+ assert (
+ json_iso_dttm_ser(np.datetime64(), pessimistic=True)
+ == "Unserializable [<class 'numpy.datetime64'>]"
+ )
with self.assertRaises(TypeError):
- json_iso_dttm_ser("this is not a date")
+ json_iso_dttm_ser(np.datetime64())
def test_base_json_conv(self):
- assert isinstance(base_json_conv(np.bool_(1)), bool) is True
- assert isinstance(base_json_conv(np.int64(1)), int) is True
- assert isinstance(base_json_conv(np.array([1, 2, 3])), list) is True
- assert isinstance(base_json_conv(set([1])), list) is True
- assert isinstance(base_json_conv(Decimal("1.0")), float) is True
- 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)
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #22029: fix: JSON serializers
Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #22029:
URL: https://github.com/apache/superset/pull/22029#discussion_r1013307678
##########
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)}")
+
-def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> str:
+def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> Any:
"""
- json serializer that deals with dates
+ A JSON serializer that deals with dates by serializing them to ISO 8601.
- >>> dttm = datetime(1970, 1, 1)
- >>> json.dumps({'dttm': dttm}, default=json_iso_dttm_ser)
- '{"dttm": "1970-01-01T00:00:00"}'
+ >>> json.dumps({'dttm': datetime(1970, 1, 1)}, default=json_iso_dttm_ser)
+ '{"dttm": "1970-01-01T00:00:00"}'
+
+ :param obj: The serializable object
+ :param pessimistic: Whether to be pessimistic regarding serialization
+ :returns: The JSON compatible form
+ :raises TypeError: If the non-pessimistic object cannot be serialized
"""
- val = base_json_conv(obj)
- if val is not None:
- return val
+
if isinstance(obj, (datetime, date, pd.Timestamp)):
- obj = obj.isoformat()
- else:
+ return obj.isoformat()
+
+ try:
+ return base_json_conv(obj)
+ except TypeError as ex:
if pessimistic:
- return "Unserializable [{}]".format(type(obj))
+ return f"Unserializable [{type(obj)}]"
- raise TypeError("Unserializable object {} of type {}".format(obj, type(obj)))
- return obj
+ raise ex
-def pessimistic_json_iso_dttm_ser(obj: Any) -> str:
+def pessimistic_json_iso_dttm_ser(obj: Any) -> Any:
"""Proxy to call json_iso_dttm_ser in a pessimistic way
If one of object is not serializable to json, it will still succeed"""
return json_iso_dttm_ser(obj, pessimistic=True)
-def json_int_dttm_ser(obj: Any) -> float:
- """json serializer that deals with dates"""
- val = base_json_conv(obj)
- if val is not None:
- return val
+def json_int_dttm_ser(obj: Any) -> Any:
+ """
+ A JSON serializer that deals with dates by serializing them to EPOCH.
+
+ >>> json.dumps({'dttm': datetime(1970, 1, 1)}, default=json_int_dttm_ser)
+ '{"dttm": 0}'
+
+ :param obj: The serializable object
+ :returns: The JSON compatible form
+ :raises TypeError: If the object cannot be serialized
+ """
+
if isinstance(obj, (datetime, pd.Timestamp)):
- obj = datetime_to_epoch(obj)
- elif isinstance(obj, date):
- obj = (obj - EPOCH.date()).total_seconds() * 1000
- else:
- raise TypeError("Unserializable object {} of type {}".format(obj, type(obj)))
- return obj
+ return datetime_to_epoch(obj)
Review Comment:
See above.
##########
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
with self.assertRaises(TypeError):
- json_iso_dttm_ser("this is not a date")
+ json_iso_dttm_ser(np.datetime64())
def test_base_json_conv(self):
- assert isinstance(base_json_conv(np.bool_(1)), bool) is True
- assert isinstance(base_json_conv(np.int64(1)), int) is True
- assert isinstance(base_json_conv(np.array([1, 2, 3])), list) is True
- assert isinstance(base_json_conv(set([1])), list) is True
- assert isinstance(base_json_conv(Decimal("1.0")), float) is True
- 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)
+ 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
Review Comment:
This is a new test.
##########
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
Review Comment:
The non-temporal case was never tested.
##########
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
with self.assertRaises(TypeError):
- json_iso_dttm_ser("this is not a date")
+ json_iso_dttm_ser(np.datetime64())
def test_base_json_conv(self):
- assert isinstance(base_json_conv(np.bool_(1)), bool) is True
- assert isinstance(base_json_conv(np.int64(1)), int) is True
- assert isinstance(base_json_conv(np.array([1, 2, 3])), list) is True
- assert isinstance(base_json_conv(set([1])), list) is True
- assert isinstance(base_json_conv(Decimal("1.0")), float) is True
- 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)
+ 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
+ assert isinstance(base_json_conv(set([1])), list)
+ assert isinstance(base_json_conv(Decimal("1.0")), float)
+ assert isinstance(base_json_conv(uuid.uuid4()), str)
+ assert isinstance(base_json_conv(time()), str)
+ assert isinstance(base_json_conv(timedelta(0)), str)
+ assert isinstance(base_json_conv(bytes()), str)
+ assert base_json_conv(bytes("", encoding="utf-16")) == ["bytes"]
+
+ with pytest.raises(TypeError):
Review Comment:
This is a new test.
##########
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)}")
+
-def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> str:
+def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> Any:
"""
- json serializer that deals with dates
+ A JSON serializer that deals with dates by serializing them to ISO 8601.
- >>> dttm = datetime(1970, 1, 1)
- >>> json.dumps({'dttm': dttm}, default=json_iso_dttm_ser)
- '{"dttm": "1970-01-01T00:00:00"}'
+ >>> json.dumps({'dttm': datetime(1970, 1, 1)}, default=json_iso_dttm_ser)
+ '{"dttm": "1970-01-01T00:00:00"}'
+
+ :param obj: The serializable object
+ :param pessimistic: Whether to be pessimistic regarding serialization
+ :returns: The JSON compatible form
+ :raises TypeError: If the non-pessimistic object cannot be serialized
"""
- val = base_json_conv(obj)
- if val is not None:
- return val
+
if isinstance(obj, (datetime, date, pd.Timestamp)):
- obj = obj.isoformat()
- else:
+ return obj.isoformat()
+
+ try:
Review Comment:
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.
##########
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)}")
+
-def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> str:
+def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> Any:
Review Comment:
The return type was wrong. Per the `base_json_conv` method there's a slew of viable return types.
##########
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)}")
+
-def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> str:
+def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> Any:
"""
- json serializer that deals with dates
+ A JSON serializer that deals with dates by serializing them to ISO 8601.
- >>> dttm = datetime(1970, 1, 1)
- >>> json.dumps({'dttm': dttm}, default=json_iso_dttm_ser)
- '{"dttm": "1970-01-01T00:00:00"}'
+ >>> json.dumps({'dttm': datetime(1970, 1, 1)}, default=json_iso_dttm_ser)
+ '{"dttm": "1970-01-01T00:00:00"}'
+
+ :param obj: The serializable object
+ :param pessimistic: Whether to be pessimistic regarding serialization
+ :returns: The JSON compatible form
+ :raises TypeError: If the non-pessimistic object cannot be serialized
"""
- val = base_json_conv(obj)
- if val is not None:
- return val
+
if isinstance(obj, (datetime, date, pd.Timestamp)):
- obj = obj.isoformat()
- else:
+ return obj.isoformat()
+
+ try:
+ return base_json_conv(obj)
+ except TypeError as ex:
if pessimistic:
- return "Unserializable [{}]".format(type(obj))
+ return f"Unserializable [{type(obj)}]"
- raise TypeError("Unserializable object {} of type {}".format(obj, type(obj)))
- return obj
+ raise ex
-def pessimistic_json_iso_dttm_ser(obj: Any) -> str:
+def pessimistic_json_iso_dttm_ser(obj: Any) -> Any:
Review Comment:
The return type was wrong. Per the `base_json_conv` method there's a slew of viable return types.
##########
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)}")
Review Comment:
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.
##########
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
Review Comment:
The non-temporal case was never tested.
##########
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
with self.assertRaises(TypeError):
- json_iso_dttm_ser("this is not a date")
+ json_iso_dttm_ser(np.datetime64())
def test_base_json_conv(self):
- assert isinstance(base_json_conv(np.bool_(1)), bool) is True
- assert isinstance(base_json_conv(np.int64(1)), int) is True
- assert isinstance(base_json_conv(np.array([1, 2, 3])), list) is True
- assert isinstance(base_json_conv(set([1])), list) is True
- assert isinstance(base_json_conv(Decimal("1.0")), float) is True
- 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)
Review Comment:
The `is True` is unnecessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #22029: fix: JSON serializers
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #22029:
URL: https://github.com/apache/superset/pull/22029#discussion_r1013354535
##########
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)}")
+
-def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> str:
+def json_iso_dttm_ser(obj: Any, pessimistic: bool = False) -> Any:
"""
- json serializer that deals with dates
+ A JSON serializer that deals with dates by serializing them to ISO 8601.
- >>> dttm = datetime(1970, 1, 1)
- >>> json.dumps({'dttm': dttm}, default=json_iso_dttm_ser)
- '{"dttm": "1970-01-01T00:00:00"}'
+ >>> json.dumps({'dttm': datetime(1970, 1, 1)}, default=json_iso_dttm_ser)
+ '{"dttm": "1970-01-01T00:00:00"}'
+
+ :param obj: The serializable object
+ :param pessimistic: Whether to be pessimistic regarding serialization
+ :returns: The JSON compatible form
+ :raises TypeError: If the non-pessimistic object cannot be serialized
"""
- val = base_json_conv(obj)
- if val is not None:
- return val
+
if isinstance(obj, (datetime, date, pd.Timestamp)):
- obj = obj.isoformat()
- else:
+ return obj.isoformat()
+
+ try:
+ return base_json_conv(obj)
+ except TypeError as ex:
if pessimistic:
- return "Unserializable [{}]".format(type(obj))
+ return f"Unserializable [{type(obj)}]"
- raise TypeError("Unserializable object {} of type {}".format(obj, type(obj)))
- return obj
+ raise ex
-def pessimistic_json_iso_dttm_ser(obj: Any) -> str:
+def pessimistic_json_iso_dttm_ser(obj: Any) -> Any:
"""Proxy to call json_iso_dttm_ser in a pessimistic way
If one of object is not serializable to json, it will still succeed"""
return json_iso_dttm_ser(obj, pessimistic=True)
-def json_int_dttm_ser(obj: Any) -> float:
- """json serializer that deals with dates"""
- val = base_json_conv(obj)
- if val is not None:
- return val
+def json_int_dttm_ser(obj: Any) -> Any:
+ """
+ A JSON serializer that deals with dates by serializing them to EPOCH.
+
+ >>> json.dumps({'dttm': datetime(1970, 1, 1)}, default=json_int_dttm_ser)
+ '{"dttm": 0}'
+
+ :param obj: The serializable object
+ :returns: The JSON compatible form
+ :raises TypeError: If the object cannot be serialized
+ """
+
if isinstance(obj, (datetime, pd.Timestamp)):
- obj = datetime_to_epoch(obj)
- elif isinstance(obj, date):
- obj = (obj - EPOCH.date()).total_seconds() * 1000
- else:
- raise TypeError("Unserializable object {} of type {}".format(obj, type(obj)))
- return obj
+ return datetime_to_epoch(obj)
+
+ if isinstance(obj, date):
+ return (obj - EPOCH.date()).total_seconds() * 1000
Review Comment:
i think we can use [`obj.timestamp()`](https://docs.python.org/3/library/datetime.html#datetime.datetime.timestamp) now since we are on python 3.3+.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org