config: allow choosing separator character and encoding for csv writing #3441

JulieRossi · 2017-09-11T08:47:16Z

This PR adds two global variables in config.py, CSV_SEP and CSV_ENCODING allowing to configure at application level, which separator character and which encoding should be used to write a csv file.

Fixes #1519

xrmx · 2017-09-11T08:53:16Z

superset/views/core.py

@@ -2126,13 +2126,15 @@ def csv(self, client_id):
            columns = [c['name'] for c in obj['columns']]
            df = pd.DataFrame.from_records(obj['data'], columns=columns)
            logging.info("Using pandas to convert to CSV")
-            csv = df.to_csv(index=False, encoding='utf-8')
+            csv = df.to_csv(index=False, encoding=str(config.get('CSV_ENCODING')),


why calling str() on the options?

To re-cast the string, otherwise I get TypeError: "delimiter" must be string, not unicode with python 2.x
If you have a better solution, I can change that (or maybe add comment to clarify inside the code ?)

Should be fine then

xrmx · 2017-09-11T08:55:25Z

superset/config.py

@@ -172,6 +172,9 @@
 ENABLE_CORS = False
 CORS_OPTIONS = {}

+# CSV Options
+CSV_SEP = ','


Since CSV has a ton of options and pandas does not support dialects i think a CSV_EXPORT dict would be more future proof:

CSV_EXPORT = { 'SEPARATOR': ',', 'ENCODING': 'utf-8', }

Indeed, this looks better. I will implement it this way.

Just waiting until you tell what you prefer regarding the previous comment about str() and I update my PR.

Actually, I was thinking I can do something more generic to be able to replace any option of the to_csv method. I'm gonna change this and resubmit.

coveralls · 2017-09-11T09:08:10Z

Coverage increased (+0.009%) to 69.127% when pulling 12c8ffbf86d7b3749376accb8ed89df532c60ecf on JulieRossi:master into 3dfdde1 on apache:master.

xrmx · 2017-09-11T09:14:44Z

Are csv exports covered by tests? If not would be nice to add them :)

JulieRossi · 2017-09-11T09:33:33Z

test_csv_endpoint (https://github.com/apache/incubator-superset/blob/master/tests/core_tests.py#L527)
tests csv export route (https://github.com/apache/incubator-superset/blob/master/superset/views/core.py#L2102)

test_slice_csv_endpoint (https://github.com/apache/incubator-superset/blob/master/tests/core_tests.py#L92)
tests csv export from slice (https://github.com/apache/incubator-superset/blob/master/superset/viz.py#L306)

So both cases are already tested.

coveralls · 2017-09-11T09:36:20Z

Coverage increased (+0.005%) to 69.123% when pulling 2af618e27bdcc24863def4f1c896bc49c6d30e71 on JulieRossi:master into 3dfdde1 on apache:master.

mistercrunch · 2017-09-11T18:02:19Z

superset/views/core.py

@@ -2126,13 +2126,15 @@ def csv(self, client_id):
            columns = [c['name'] for c in obj['columns']]
            df = pd.DataFrame.from_records(obj['data'], columns=columns)
            logging.info("Using pandas to convert to CSV")
-            csv = df.to_csv(index=False, encoding='utf-8')
+            csv = df.to_csv(index=False, encoding=str(config.get('CSV_EXPORT')['ENCODING']),
+                            sep=str(config.get('CSV_EXPORT')['SEPARATOR']))


Why casting to str?

To re-cast the string, otherwise I get TypeError: "delimiter" must be string, not unicode with python 2.x
If you have a better solution, I can change that (or maybe add comment to clarify inside the code ?)

mistercrunch · 2017-09-11T18:02:24Z

superset/views/core.py

        else:
            logging.info("Running a query to turn into CSV")
            sql = query.select_sql or query.executed_sql
            df = query.database.get_df(sql, query.schema)
            # TODO(bkyryliuk): add compression=gzip for big files.
-            csv = df.to_csv(index=False, encoding='utf-8')
+            csv = df.to_csv(index=False, encoding=str(config.get('CSV_EXPORT')['ENCODING']),


Why casting to str?

To re-cast the string, otherwise I get TypeError: "delimiter" must be string, not unicode with python 2.x
If you have a better solution, I can change that (or maybe add comment to clarify inside the code ?)

mistercrunch · 2017-09-11T18:02:33Z

superset/viz.py

@@ -306,7 +306,8 @@ def data(self):
    def get_csv(self):
        df = self.get_df()
        include_index = not isinstance(df.index, pd.RangeIndex)
-        return df.to_csv(index=include_index, encoding="utf-8")
+        return df.to_csv(index=include_index, encoding=str(config.get('CSV_EXPORT')['ENCODING']),


Why casting to str?

To re-cast the string, otherwise I get TypeError: "delimiter" must be string, not unicode with python 2.x
If you have a better solution, I can change that (or maybe add comment to clarify inside the code ?)

mistercrunch · 2017-09-11T18:04:34Z

Is there a use case to not use utf-8? Changing the delimiter globally seems like a bad idea as the C in CSV stands for comma...

JulieRossi · 2017-09-12T07:55:56Z

Regarding the non utf-8 encoding and the delimiter, we have the same problem kylozw describes in #1519.
Our users are used to Excel so when they export csv the want to be able to easily open it with Excel (they are not advanced users and changing Excel default encoding and delimiter is beyond their knowledge).
In the french version, Excel expects latin1 encoding and ; as separator (also , as decimal separator instead of .). It seems that others have the same kind of issues (given #1519) so I thought issuing a PR was a better idea than just changing this locally. It would also be more convenient for us.

I also commented (but probably not visible enough since it was just a reply to a former comment) that I was thinking I can do something more generic to be able to replace any option of the to_csv method. I'm gonna change this and resubmit.

coveralls · 2017-09-12T18:00:15Z

Coverage increased (+0.005%) to 69.124% when pulling dfed319 on JulieRossi:master into 3c0e85e on apache:master.

coveralls · 2017-09-12T18:02:07Z

Coverage increased (+0.005%) to 69.124% when pulling dfed319 on JulieRossi:master into 3c0e85e on apache:master.

coveralls · 2017-09-12T18:02:07Z

Coverage increased (+0.005%) to 69.124% when pulling dfed319 on JulieRossi:master into 3c0e85e on apache:master.

coveralls · 2017-09-12T18:02:07Z

Coverage increased (+0.005%) to 69.124% when pulling dfed319 on JulieRossi:master into 3c0e85e on apache:master.

xrmx suggested changes Sep 11, 2017

View reviewed changes

mistercrunch reviewed Sep 11, 2017

View reviewed changes

config: allow changing default options for writting csv

dfed319

mistercrunch merged commit b90d8e3 into apache:master Sep 15, 2017

ashdaka mentioned this pull request Sep 18, 2017

Choose encoding when export csv #1519

Closed

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.20.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: allow choosing separator character and encoding for csv writing #3441

config: allow choosing separator character and encoding for csv writing #3441

JulieRossi commented Sep 11, 2017

xrmx Sep 11, 2017

JulieRossi Sep 11, 2017

xrmx Sep 11, 2017

xrmx Sep 11, 2017

JulieRossi Sep 11, 2017 •

edited

JulieRossi Sep 11, 2017

coveralls commented Sep 11, 2017 •

edited

xrmx commented Sep 11, 2017

JulieRossi commented Sep 11, 2017

coveralls commented Sep 11, 2017 •

edited

mistercrunch Sep 11, 2017

JulieRossi Sep 12, 2017

mistercrunch Sep 11, 2017

JulieRossi Sep 12, 2017

mistercrunch Sep 11, 2017

JulieRossi Sep 12, 2017

mistercrunch commented Sep 11, 2017

JulieRossi commented Sep 12, 2017 •

edited

coveralls commented Sep 12, 2017 •

edited

coveralls commented Sep 12, 2017 •

edited

coveralls commented Sep 12, 2017

coveralls commented Sep 12, 2017

config: allow choosing separator character and encoding for csv writing #3441

config: allow choosing separator character and encoding for csv writing #3441

Conversation

JulieRossi commented Sep 11, 2017

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

JulieRossi Sep 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 11, 2017 • edited

xrmx commented Sep 11, 2017

JulieRossi commented Sep 11, 2017

coveralls commented Sep 11, 2017 • edited

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

mistercrunch commented Sep 11, 2017

JulieRossi commented Sep 12, 2017 • edited

coveralls commented Sep 12, 2017 • edited

coveralls commented Sep 12, 2017 • edited

coveralls commented Sep 12, 2017

coveralls commented Sep 12, 2017

JulieRossi Sep 11, 2017 •

edited

coveralls commented Sep 11, 2017 •

edited

coveralls commented Sep 11, 2017 •

edited

JulieRossi commented Sep 12, 2017 •

edited

coveralls commented Sep 12, 2017 •

edited

coveralls commented Sep 12, 2017 •

edited