Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace NaN/Infinity with null #4908

Merged
merged 1 commit into from May 1, 2018
Merged

Replace NaN/Infinity with null #4908

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 5 additions & 4 deletions superset/views/core.py
View file
Open in desktop
Expand Up @@ -6,7 +6,6 @@
from __future__ import unicode_literals

from datetime import datetime, timedelta
import json
import logging
import os
import re
Expand All @@ -24,6 +23,7 @@
from flask_babel import gettext as __
from flask_babel import lazy_gettext as _
import pandas as pd
import simplejson as json
from six import text_type
import sqlalchemy as sqla
from sqlalchemy import create_engine
Expand Down Expand Up @@ -2327,7 +2327,8 @@ def results(self, key):
payload_json = json.loads(payload)
payload_json['data'] = payload_json['data'][:display_limit]
return json_success(
json.dumps(payload_json, default=utils.json_iso_dttm_ser))
json.dumps(
payload_json, default=utils.json_iso_dttm_ser, ignore_nan=True))

@has_access_api
@expose('/stop_query/', methods=['POST'])
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little bit scary as it was set to False explicitly in the past, probably for a reason. I'm guessing the default is True and someone set it to False for some specific reason. We should do a bit of git forensics to understand how it came to be.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Digging in a bit more, I found the first commit where this was introduced by some guy called @mistercrunch: 38b8db805 :-P

Initially both sync and async responses used allow_nan=False. Eventually it got dropped from the sync response in 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.

session.commit()
return resp

Expand All @@ -2453,7 +2454,7 @@ def sql_json(self):
rendered_query,
return_results=True)
payload = json.dumps(
data, default=utils.pessimistic_json_iso_dttm_ser)
data, default=utils.pessimistic_json_iso_dttm_ser, ignore_nan=True)
except Exception as e:
logging.exception(e)
return json_error_response('{}'.format(e))
Expand Down