You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mistercrunch (via GitHub)" <gi...@apache.org> on 2024/04/29 16:30:53 UTC
[PR] fix: use pessimistic json encoder in SQL Lab [superset]
mistercrunch opened a new pull request, #28266:
URL: https://github.com/apache/superset/pull/28266
### SUMMARY
An issue reported in https://github.com/apache/superset/issues/28001 shows SQLLab failing hard when a database driver returns a type that can't be decoded for serialization. Switching to this pessimistic helper function for json serialization will prevent a hard failure, but will obfuscate the data for the specific cells where things can't be serialized.
Not being in a situation where I can reproduce the issue, I'm operating with the stacktrace, meaning I can't really handle the specific cell, but can prevent SQL Lab from failing hard on a single cell issue.
Few related notes:
- maybe there's something around the driver being in utf8 mode of some kind. In theory we could say that driver / superset should agree on utf8 for things to work
- driver should probably giving us a `bytes` type from a `VARBINARY` and from the stacktrace it appears to be something else, presumably `str`, though that's unclear
- could be good to refactor out everything json-related out of superset/utils/core.py and into a new superset/utils/json.py, and make sure we always use thesame utility functions throughout the codebase as opposed to raw `import json` across the codebase. From my understanding there's some magic around simplejson injecting itself everywhere that happens too.
--
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
Re: [PR] fix: use pessimistic json encoder in SQL Lab [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #28266:
URL: https://github.com/apache/superset/pull/28266#discussion_r1583472032
##########
superset/sqllab/api.py:
##########
@@ -340,13 +340,15 @@ def get_results(self, **kwargs: Any) -> FlaskResponse:
key = params.get("key")
rows = params.get("rows")
result = SqlExecutionResultsCommand(key=key, rows=rows).run()
+
+ payload = json.dumps(
+ result,
+ default=utils.pessimistic_json_iso_dttm_ser,
+ ignore_nan=True,
+ )
# return the result without special encoding
return json_success(
- json.dumps(
Review Comment:
Why not keep this inline given `payload` is a single use variable.
--
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
Re: [PR] fix: use pessimistic json encoder in SQL Lab [superset]
Posted by "Habeeb556 (via GitHub)" <gi...@apache.org>.
Habeeb556 commented on PR #28266:
URL: https://github.com/apache/superset/pull/28266#issuecomment-2204291364
I have the same problem encoder here #29457 but with `TIMESTAMP` datatype.
`Version 4.0.1`
--
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
Re: [PR] fix: use pessimistic json encoder in SQL Lab [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #28266:
URL: https://github.com/apache/superset/pull/28266
--
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
Re: [PR] fix: use pessimistic json encoder in SQL Lab [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #28266:
URL: https://github.com/apache/superset/pull/28266#discussion_r1583608527
##########
superset/sqllab/api.py:
##########
@@ -340,13 +340,15 @@ def get_results(self, **kwargs: Any) -> FlaskResponse:
key = params.get("key")
rows = params.get("rows")
result = SqlExecutionResultsCommand(key=key, rows=rows).run()
+
+ payload = json.dumps(
+ result,
+ default=utils.pessimistic_json_iso_dttm_ser,
+ ignore_nan=True,
+ )
# return the result without special encoding
return json_success(
- json.dumps(
Review Comment:
on this I'm happy either way, but thought the change made things a bit more readable, maybe making it a tiny bit easier to read the stacktrace or debug. But I don't have strong opinions on single-use variable
--
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
Re: [PR] fix: use pessimistic json encoder in SQL Lab [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #28266:
URL: https://github.com/apache/superset/pull/28266#issuecomment-2083446038
Digging a bit it appears the pessimistic serializer would fill the unserializable cell with `f"Unserializable [{type(obj)}]"` https://github.com/apache/superset/blob/master/superset/utils/core.py#L508-L510 , which seems reasonable.
Though maybe catching `TypeError` is not broad enough here too as I think given the diversity and maturity spectrum of drivers we deal with, we should assume that it's likely we might get objects we cannot serialize and should fail gracefully (let bot user know and leave a good trace in the logs)
So left TODO here:
- try and get more info from the issue reported as to what type isn't handed,
- handle that specific type as gracefully as possible
- write a unit test specific to that type
- assume we may get ANY object from a driver
- write a unit test representing a fully "unhandlable" type
- make sure we log something useful
- fill in the cell something that makes some sense to the user
--
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
Re: [PR] fix: use pessimistic json encoder in SQL Lab [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #28266:
URL: https://github.com/apache/superset/pull/28266#issuecomment-2083417816
+1 totally agree on the unit test + logging something to the user and probably logging something to the administrator. Marking this as DRAFT as it needs more work, but want to share potential solution with the person who reported the issue.
--
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
Re: [PR] fix: use pessimistic json encoder in SQL Lab [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #28266:
URL: https://github.com/apache/superset/pull/28266#issuecomment-2206722114
@Habeeb556 looks like 4.0.2 brings this cheery in if that helps.
--
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
Re: [PR] fix: use pessimistic json encoder in SQL Lab [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #28266:
URL: https://github.com/apache/superset/pull/28266#issuecomment-2083303991
@mistercrunch Should we show some kind of warning to the users when we obfuscate data? I'm trying to avoid a scenario where data is missing because of quality issues and users are not aware of the problem.
--
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
Re: [PR] fix: use pessimistic json encoder in SQL Lab [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch closed pull request #28266: fix: use pessimistic json encoder in SQL Lab
URL: https://github.com/apache/superset/pull/28266
--
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