[GitHub] betodealmeida commented on a change in pull request #4908: Replace NaN/Infinity with null

[GitBox]

betodealmeida commented on a change in pull request #4908: Replace NaN/Infinity 
with null
URL: 
https://github.com/apache/incubator-superset/pull/4908#discussion_r185292674
 
 

 ##
 File path: superset/views/core.py
 ##
 @@ -2435,7 +2436,7 @@ def sql_json(self):
 
 resp = json_success(json.dumps(
 {'query': query.to_dict()}, default=utils.json_int_dttm_ser,
-allow_nan=False), status=202)
+ignore_nan=True), status=202)
 
 Review comment:
   Digging in a bit more, I found the first commit where this was introduced by 
some guy called @mistercrunch: 
https://github.com/apache/incubator-superset/commit/38b8db805 :-P
   
   Initially both sync and async responses used `allow_nan=False`. Eventually 
it got dropped from the sync response in 
https://github.com/apache/incubator-superset/commit/269f55c29 when the 
pessimistic encoder was introduced. The pessimistic encoder was not added to 
the async response because the commit is trying to fix gigantic HTML error 
messages, but IMHO it should've been added as well.
   
   The problem is that the pessimistic encoder doesn't handle `NaN` and 
`±Infinity` because they're floats, so the JSON encoder never calls the 
`default` method on them.
   
   I think the change above is safe — it will return _some_ result instead of 
failing with a `ValueError` exception. But I think the best approach would be 
using a more robust serialization to send data to the browser, like BSON.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] betodealmeida commented on a change in pull request #4908: Replace NaN/Infinity with null

[GitBox]

betodealmeida commented on a change in pull request #4908: Replace NaN/Infinity 
with null
URL: 
https://github.com/apache/incubator-superset/pull/4908#discussion_r185049884
 
 

 ##
 File path: superset/views/core.py
 ##
 @@ -2435,7 +2436,7 @@ def sql_json(self):
 
 resp = json_success(json.dumps(
 {'query': query.to_dict()}, default=utils.json_int_dttm_ser,
-allow_nan=False), status=202)
+ignore_nan=True), status=202)
 
 Review comment:
   Yeah. This is super confusing naming. Just to clarify:
   
   - `allow_nan=False`: will raise an exception if the data has NaN/Infinity;
   - `ignore_nan=True`: will encode NaN/Infinity as nulls.
   
   I'm assuming what happened was that someone had an async query returning 
NaN/Infinity, and since the stdlib `json` module does not have the `ignore_nan` 
argument the easiest way is to work around is to raise an exception here. It 
took me a while to learn that `simplejson` had this option.
   
   I looked at the blame and couldn't find any info about this. The reason I 
changed this as well was to make it consistent with the sync call.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services