You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "squalou (via GitHub)" <gi...@apache.org> on 2024/02/23 13:01:30 UTC
[PR] #23375 set columns numeric datatypes when exporting to excel [superset]
squalou opened a new pull request, #27229:
URL: https://github.com/apache/superset/pull/27229
### SUMMARY
When exporting to Excel, currently datatypes are not specified, leading to decimal numbers being stored as strings, which may lead to issues when opening the file.
Depending on the locale, the issue may not even be visible, Excel managing to convert things. (that's the case in us/en locale). When using another locale (say fr), then numbers, output with '.' as decimal separator and stored as strings won't be usable as numbers in Excel.
The idea here is to use the same "datatype guessing" method already existing, and use it to convert dataframe columns types when required before exporting.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
not applicable
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
- Create a Table dashboard with numercical and decimal numbers in it. Ideally add some strings and dates.
- Export to Excel
- you can then unzip the .xlsx file, and open sheet1.xml in a text editor to check the export
- Numbers appear directly in xml cell reference
- Strings are not visible directly, instead a `<s>` markup is used containing an id to the string
Editing the sheet1.xml file is the safest way to check the issue, due to magical operations softwares like Excel, LibreOffice Calc or others perform when opening files that may hide the issue.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: Fixes #23375
- [ ] 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
Re: [PR] fix: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-2000043476
I've been away a bit, I'm back. Is there anything I should do ?
Maybe try a rebase master and shoot again ?
right now I'll wait for further instructions, rather than adding mess to the situation :)
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-2006111056
> Hah... I'm rerunning the failing test now in hopes that it's just flaky. Fingers crossed.
damn, from what I remember previous crash was not this one. (do we have history of previous run somewhere to be sure?)
```
SqlLab query tabs
We detected that the Chromium Renderer process just crashed.
```
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #27229:
URL: https://github.com/apache/superset/pull/27229#discussion_r1516145763
##########
superset/utils/excel.py:
##########
@@ -19,16 +19,36 @@
import pandas as pd
+from superset.utils.core import GenericDataType
-def df_to_excel(df: pd.DataFrame, **kwargs: Any) -> Any:
+
+def df_to_excel(
+ df: pd.DataFrame, coltypes: list[GenericDataType], **kwargs: Any
+) -> Any:
output = io.BytesIO()
# timezones are not supported
for column in df.select_dtypes(include=["datetimetz"]).columns:
df[column] = df[column].astype(str)
+ ndf = copy_df_with_datatype_conversion(df, coltypes)
Review Comment:
@squalou Can the column types be extracted from the `DataFrame` like we are doing for timezones
```
for column in df.select_dtypes(include=["datetimetz"]).columns:
df[column] = df[column].astype(str)
```
or are they not matching what's defined in `coltypes`?
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-1989690651
> hm, no idea how this could be related with the PR, could it be failing on master before the changes ?
Agreed, seems unrelated... I'll rerun it in hopes that it's just a flaky test.
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #27229:
URL: https://github.com/apache/superset/pull/27229#discussion_r1516268116
##########
tests/unit_tests/utils/excel_tests.py:
##########
@@ -18,14 +18,74 @@
from datetime import datetime, timezone
import pandas as pd
+from pandas.api.types import is_numeric_dtype
-from superset.utils.excel import df_to_excel
+from superset.utils.core import GenericDataType
+from superset.utils.excel import convert_df_with_datatypes, df_to_excel
def test_timezone_conversion() -> None:
"""
Test that columns with timezones are converted to a string.
"""
df = pd.DataFrame({"dt": [datetime(2023, 1, 1, 0, 0, tzinfo=timezone.utc)]})
- contents = df_to_excel(df)
+ contents = df_to_excel(df, [])
Review Comment:
```suggestion
contents = df_to_excel(df, [GenericDataType.TEMPORAL])
```
##########
superset/common/query_context_processor.py:
##########
@@ -571,11 +574,22 @@ def get_data(self, df: pd.DataFrame) -> str | list[dict[str, Any]]:
df, index=include_index, **config["CSV_EXPORT"]
)
elif self._query_context.result_format == ChartDataResultFormat.XLSX:
- result = excel.df_to_excel(df, **config["EXCEL_EXPORT"])
+ ndf = self.copy_df_for_excel_export(df, coltypes)
Review Comment:
I checked that `coltypes` are extracted from the `DataFrame` [here](https://github.com/apache/superset/blob/ce0b70cc8685aa4d83c0c4ad0fb8f03fb7e28ede/superset/common/query_actions.py#L109).
##########
superset/utils/excel.py:
##########
@@ -19,16 +19,36 @@
import pandas as pd
+from superset.utils.core import GenericDataType
-def df_to_excel(df: pd.DataFrame, **kwargs: Any) -> Any:
+
+def df_to_excel(
+ df: pd.DataFrame, coltypes: list[GenericDataType], **kwargs: Any
+) -> Any:
output = io.BytesIO()
# timezones are not supported
for column in df.select_dtypes(include=["datetimetz"]).columns:
df[column] = df[column].astype(str)
+ ndf = convert_df_with_datatypes(df, coltypes)
+
# pylint: disable=abstract-class-instantiated
with pd.ExcelWriter(output, engine="xlsxwriter") as writer:
- df.to_excel(writer, **kwargs)
+ ndf.to_excel(writer, **kwargs)
return output.getvalue()
+
+
+def convert_df_with_datatypes(
Review Comment:
I believe we could make it more explicit that the `DataFrame` can be modified and avoid the copy overhead. To do that, we need to separate column conversion from exporting. Something like:
```
def df_to_excel(df: pd.DataFrame, **kwargs: Any) -> bytes:
output = io.BytesIO()
# pylint: disable=abstract-class-instantiated
with pd.ExcelWriter(output, engine="xlsxwriter") as writer:
df.to_excel(writer, **kwargs)
return output.getvalue()
def apply_column_types(
df: pd.DataFrame, column_types: list[GenericDataType]
) -> pd.DataFrame:
for column, column_type in zip(df.columns, column_types):
if column_type == GenericDataType.NUMERIC:
df[column] = pd.to_numeric(df[column])
elif pd.api.types.is_datetime64tz_dtype(df[column]):
# timezones are not supported
df[column] = df[column].astype(str)
else:
df[column] = df[column].astype(column_type)
return df
```
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-1961923884
Thank you for the fix @squalou. Could you add a test for it to avoid future regressions?
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #27229:
URL: https://github.com/apache/superset/pull/27229#discussion_r1514983244
##########
superset/common/query_context_processor.py:
##########
@@ -571,11 +574,22 @@ def get_data(self, df: pd.DataFrame) -> str | list[dict[str, Any]]:
df, index=include_index, **config["CSV_EXPORT"]
)
elif self._query_context.result_format == ChartDataResultFormat.XLSX:
- result = excel.df_to_excel(df, **config["EXCEL_EXPORT"])
+ ndf = self.copy_df_for_excel_export(df, coltypes)
Review Comment:
@squalou Should this change be actually implemented in `superset.utils.excel.df_to_excel` to ensure consistent behavior? You could move your tests to `tests/unit_tests/utils/excel_tests`. We already have one there called `test_timezone_conversion` which is a similar type of conversion.
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on code in PR #27229:
URL: https://github.com/apache/superset/pull/27229#discussion_r1515199324
##########
superset/common/query_context_processor.py:
##########
@@ -571,11 +574,22 @@ def get_data(self, df: pd.DataFrame) -> str | list[dict[str, Any]]:
df, index=include_index, **config["CSV_EXPORT"]
)
elif self._query_context.result_format == ChartDataResultFormat.XLSX:
- result = excel.df_to_excel(df, **config["EXCEL_EXPORT"])
+ ndf = self.copy_df_for_excel_export(df, coltypes)
Review Comment:
Makes a lot of sense, sure.
What would be better,
add "coltypes" as optional parameter in excel.df_to_excel(...) method ?
Or move the method as-is in superset.utils.excel. package.
First option seems better but I'm not at ease with changing method signatures there without an advice, I really have no global overview of what may call what and consequences.
EDIT : from a quick check the method is called in one place and one test so ... let's go.
--
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] #23375 set columns numeric datatypes when exporting to excel [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-1961904015
Approving CI run🤞
--
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(23375): set columns numeric datatypes when exporting to excel [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-1961920198
Looks like you need to run the [pre-commit hook](https://superset.apache.org/docs/contributing/hooks-and-linting/) for formatting/linting.
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-1983995465
damn, I still have a minor pylint issue ("exception as e" as a bad habit, moreover e is not used :) )
I have an issue though with an error in a file I did not change
```
superset/commands/base.py:61:0: I0021: Useless suppression of 'too-few-public-methods' (useless-suppression)
```
that's weird.
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-1962312284
> Looks like you need to run the [pre-commit hook](https://superset.apache.org/docs/contributing/hooks-and-linting/) for formatting/linting.
yeah sorry about that. Should be fixed, my IDE for this project is not on "auto format", totally forgot.
> Thank you for the fix @squalou. Could you add a test for it to avoid future regressions?
I'd love to ! really. But I admit I' a bit lost in superset architecture and test framework, I'm not a python pro dev to begin with which does not help.
I've tried to guess where 'export csv' might be tested and enhance from there but did not find anything. Got myself lost in viz_tests.py and didn't go any further.
Any help / pointer to where to begin with would be appreciated.
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-2224123296
I just fixed the conflict for you, so I think that should fix CI.
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on code in PR #27229:
URL: https://github.com/apache/superset/pull/27229#discussion_r1516538840
##########
superset/utils/excel.py:
##########
@@ -19,16 +19,36 @@
import pandas as pd
+from superset.utils.core import GenericDataType
-def df_to_excel(df: pd.DataFrame, **kwargs: Any) -> Any:
+
+def df_to_excel(
+ df: pd.DataFrame, coltypes: list[GenericDataType], **kwargs: Any
+) -> Any:
output = io.BytesIO()
# timezones are not supported
for column in df.select_dtypes(include=["datetimetz"]).columns:
df[column] = df[column].astype(str)
+ ndf = convert_df_with_datatypes(df, coltypes)
+
# pylint: disable=abstract-class-instantiated
with pd.ExcelWriter(output, engine="xlsxwriter") as writer:
- df.to_excel(writer, **kwargs)
+ ndf.to_excel(writer, **kwargs)
return output.getvalue()
+
+
+def convert_df_with_datatypes(
Review Comment:
I'm probably over-conservative but I have two concerns
- `pd.to_numeric` may raise exception, from documentation the "ignore error" param is deprecated and it's recommended to catch exceptions.
If everything is coherent from end to end, of course conversion will succeed. If you're confident it's the case and try/catch is useless that's fine for me. Else changing df in place is an issue.
- `else : df[column] = df[column].astype(column_type)` ... again I'm a bit scared of the effect on excel export side, because I don't know how it works. Id' rather "leave untouched" data not to be converted, but here again you probably know better.
Finally if I get things right, the call to "apply_column_types" should be made in query_context_processor, right ?
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-1979323640
Hi,
I manage to run tests locally and I'm trying to add one... but I have (at last) two issues.
You'll see from these that ... I'm full of good will but a desperate python newbie :)
*First : BaseDatasource*
Following test example I initiate this
```py
query_context_processor = QueryContextProcessor(
QueryContext(
datasource=BaseDatasource(),
queries=[],
result_type=ChartDataResultType.COLUMNS,
form_data={},
slice_=None,
result_format=ChartDataResultFormat.XLSX,
cache_values={},
)
)
```
But then, in context_query_processor.py, method get_data(), theres' a call
```verbose_map = self._qc_datasource.data.get("verbose_map", {})```
that ends up calling "uid", and finally ... raise a lovely NotImplementedError. Seing the code - unless there's an object overriding teh type method somewhere - I have no idea what to do to avoid this.
```py
@property
def type(self) -> str:
raise NotImplementedError()
@property
def uid(self) -> str:
"""Unique id across datasource types"""
return f"{self.id}__{self.type}"
```
```
superset/common/query_context_processor.py:569: in get_data
verbose_map = self._qc_datasource.data.get("verbose_map", {})
superset/connectors/sqla/models.py:376: in data
"uid": self.uid,
superset/connectors/sqla/models.py:266: in uid
return f"{self.id}__{self.type}"
self = <superset.connectors.sqla.models.BaseDatasource object at 0x74ce68123fa0>
@property
def type(self) -> str:
> raise NotImplementedError()
E NotImplementedError
```
So, in order to go on a bit, I put locally a try/catch around verbose_map thing, which may be a solution eventually ?
Which leads to my even bigger issue ...
*Second : how to check*
the method I must test returns this
```py
result = excel.df_to_excel(ndf, **config["EXCEL_EXPORT"])
return result or ""
```
which is not very testable.
a log output gave me as expected a binary content that looks like this, so, yeah, the test runs.
```
2024-03-05 18:46:01,425:INFO:root:b'PK\x03\x04\x14\x00\x00\x00\x08\x00\x00\x00?\x00a]I:O\x01\x00\x00\x8f\x04\x00\x00\x13\x00\x00\x00[Content_Types].xml\xad\x94\xcbn\xc20\x10E\xf7\xfd\x8a\xc8\xdb*1tQU\x15\x81E\x1f\xcb\x16\xa9\xf4\x03\\{B,\x
```
Is there a way to "mock" or "override" the ` excel.df_to_excel` call so that the test can get comparable datas ?
thanks for any advice !
regards
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #27229:
URL: https://github.com/apache/superset/pull/27229#discussion_r1516560254
##########
superset/utils/excel.py:
##########
@@ -19,16 +19,36 @@
import pandas as pd
+from superset.utils.core import GenericDataType
-def df_to_excel(df: pd.DataFrame, **kwargs: Any) -> Any:
+
+def df_to_excel(
+ df: pd.DataFrame, coltypes: list[GenericDataType], **kwargs: Any
+) -> Any:
output = io.BytesIO()
# timezones are not supported
for column in df.select_dtypes(include=["datetimetz"]).columns:
df[column] = df[column].astype(str)
+ ndf = convert_df_with_datatypes(df, coltypes)
+
# pylint: disable=abstract-class-instantiated
with pd.ExcelWriter(output, engine="xlsxwriter") as writer:
- df.to_excel(writer, **kwargs)
+ ndf.to_excel(writer, **kwargs)
return output.getvalue()
+
+
+def convert_df_with_datatypes(
Review Comment:
> pd.to_numeric may raise exception, from documentation the "ignore error" param is deprecated and it's recommended to catch exceptions.
Good point. Here we have two options:
- Catch and convert to a default. We also need to alert the user that some values were modified when exporting.
- Don't catch and show a good message clearly indicating the column/row that is preventing the export.
> else : df[column] = df[column].astype(column_type) ... again I'm a bit scared of the effect on excel export side, because I don't know how it works. Id' rather "leave untouched" data not to be converted, but here again you probably know better.
I'm not sure what you mean by untouched data but I'm assuming that when exporting users want the data to comply with what is defined as the column type. That was your original problem for numeric types, so I'm assuming that other non-numerical columns that are not matching their defined types should also be converted.
Let me ask for @betodealmeida and @eschutho input here as they have more context on this feature and might help us.
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on code in PR #27229:
URL: https://github.com/apache/superset/pull/27229#discussion_r1515199324
##########
superset/common/query_context_processor.py:
##########
@@ -571,11 +574,22 @@ def get_data(self, df: pd.DataFrame) -> str | list[dict[str, Any]]:
df, index=include_index, **config["CSV_EXPORT"]
)
elif self._query_context.result_format == ChartDataResultFormat.XLSX:
- result = excel.df_to_excel(df, **config["EXCEL_EXPORT"])
+ ndf = self.copy_df_for_excel_export(df, coltypes)
Review Comment:
Makes a lot of sense, sure.
What would be better,
add "coltypes" as optional parameter in excel.df_to_excel(...) method ?
Or move the method as-is in superset.utils.excel. package.
First option seems better but I'm not at ease with changing method signatures there without an advice, I really have no global overview of what may call what and consequences.
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-1989455573
Python-Integration / test-mysql (3.9) (pull_request) Failing after 6m
hm, no idea how this could be related with the PR, could it be failing on master before the changes ?
```
=================================== FAILURES ===================================
__________________ TestPostChartDataApi.test_chart_data_async __________________
self = <tests.integration_tests.charts.data.api_tests.TestPostChartDataApi testMethod=test_chart_data_async>
@with_feature_flags(GLOBAL_ASYNC_QUERIES=True)
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
def test_chart_data_async(self):
self.logout()
app._got_first_request = False
async_query_manager_factory.init_app(app)
self.login("admin")
rv = self.post_assert_metric(CHART_DATA_URI, self.query_context_payload, "data")
> self.assertEqual(rv.status_code, 202)
E AssertionError: 200 != 202
tests/integration_tests/charts/data/api_tests.py:707: AssertionError
```
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-1964161512
> Any help / pointer to where to begin with would be appreciated.
You can create a test in [tests/unit_tests/common](https://github.com/apache/superset/blob/master/tests/unit_tests/common) similarly to [test_get_aggregated_join_column.py](https://github.com/apache/superset/blob/master/tests/unit_tests/common/test_get_aggregated_join_column.py)
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on code in PR #27229:
URL: https://github.com/apache/superset/pull/27229#discussion_r1516200819
##########
superset/utils/excel.py:
##########
@@ -19,16 +19,36 @@
import pandas as pd
+from superset.utils.core import GenericDataType
-def df_to_excel(df: pd.DataFrame, **kwargs: Any) -> Any:
+
+def df_to_excel(
+ df: pd.DataFrame, coltypes: list[GenericDataType], **kwargs: Any
+) -> Any:
output = io.BytesIO()
# timezones are not supported
for column in df.select_dtypes(include=["datetimetz"]).columns:
df[column] = df[column].astype(str)
+ ndf = copy_df_with_datatype_conversion(df, coltypes)
Review Comment:
Thing is : you could have data with leading zeros for instance, defined as string in the original datasource, and you want them as string in the end, or else excel would omit leading zeros, maybe round things, whatever.
I don't know much about DataFrame object, and the whole issue I'm trying to solve is to get the datatype *as it was defined* as the column type to begin with.
If you known of a way to get this data from the DataFrame itself, it sure would be MUCH more elegant than forwarding the column_types all way down to excel export.
To be honest, I discovered panda objects only to make up this patch ... so if you think this metadata can be stored / handled differently, I'll be glad to learn.
I also did not want to take any risk of changing the original dataframe object as I have no idea of global superset architecture.
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on code in PR #27229:
URL: https://github.com/apache/superset/pull/27229#discussion_r1516332226
##########
superset/utils/excel.py:
##########
@@ -19,16 +19,36 @@
import pandas as pd
+from superset.utils.core import GenericDataType
-def df_to_excel(df: pd.DataFrame, **kwargs: Any) -> Any:
+
+def df_to_excel(
+ df: pd.DataFrame, coltypes: list[GenericDataType], **kwargs: Any
+) -> Any:
output = io.BytesIO()
# timezones are not supported
for column in df.select_dtypes(include=["datetimetz"]).columns:
df[column] = df[column].astype(str)
+ ndf = copy_df_with_datatype_conversion(df, coltypes)
Review Comment:
I almost forgot the most important part : I have absolutely no idea how to know what dtypes are 'stored' in the DataFrame at the beginning :) so I used the 'coltypes' that seemed to be there for a reason :)
--
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] #23375 set columns numeric datatypes when exporting to excel [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-1961910196
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/27229?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 59.54%. Comparing base [(`c78ea20`)](https://app.codecov.io/gh/apache/superset/commit/c78ea20bef82bfc10f9fc5b9010980a979848659?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`e0df5dd`)](https://app.codecov.io/gh/apache/superset/pull/27229?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 3 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #27229 +/- ##
==========================================
- Coverage 69.46% 59.54% -9.92%
==========================================
Files 1905 1905
Lines 74580 74586 +6
Branches 8330 8330
==========================================
- Hits 51806 44415 -7391
- Misses 20724 28121 +7397
Partials 2050 2050
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/27229/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [mysql](https://app.codecov.io/gh/apache/superset/pull/27229/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/27229/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/27229/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.76% <100.00%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/27229/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.28% <100.00%> (-20.59%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/27229/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/27229/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.46% <20.00%> (-0.01%)` | :arrow_down: |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/27229?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-2305850762
Not sure why the mysql test is still flaky here, but I'll close/reopen to kick it again. Hopefully we can (finally!) get this across the finish line.
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-2308192792
> Wow, we really fell off the wagon on this one. @squalou if you want to rebase, I'm optimistic we can get this through. CI has stabilized quite a bit lately, and it seems the conflicts might not be too crazy.
Sorry, totally missed the notification, and thank you @eschutho for the rebase !
(and many thanks to all the others too for giving this long story a happy end :) )
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #27229:
URL: https://github.com/apache/superset/pull/27229
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-2007386274
> We detected that the Chromium Renderer process just crashed.
It seems that GitHub Actions runners are random machines from different clusters... these things are hard to nail down, but certain machines don't match, and have troubles. It makes the tests look flaky, which is really annoying. I'm re-running that 1/3 of the E2E tests now 🤞
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-1985191842
damn ! my "black" local version doesn't behave exactly the same as the one on CI ! Will be fixed on next run.
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on code in PR #27229:
URL: https://github.com/apache/superset/pull/27229#discussion_r1516538840
##########
superset/utils/excel.py:
##########
@@ -19,16 +19,36 @@
import pandas as pd
+from superset.utils.core import GenericDataType
-def df_to_excel(df: pd.DataFrame, **kwargs: Any) -> Any:
+
+def df_to_excel(
+ df: pd.DataFrame, coltypes: list[GenericDataType], **kwargs: Any
+) -> Any:
output = io.BytesIO()
# timezones are not supported
for column in df.select_dtypes(include=["datetimetz"]).columns:
df[column] = df[column].astype(str)
+ ndf = convert_df_with_datatypes(df, coltypes)
+
# pylint: disable=abstract-class-instantiated
with pd.ExcelWriter(output, engine="xlsxwriter") as writer:
- df.to_excel(writer, **kwargs)
+ ndf.to_excel(writer, **kwargs)
return output.getvalue()
+
+
+def convert_df_with_datatypes(
Review Comment:
I'm probably over-conservative but I have one remaining concern
- `else : df[column] = df[column].astype(column_type)` ... again I'm a bit scared of the effect on excel export side, because I don't know how it works. Id' rather "leave untouched" data not to be converted, but here again you probably know better.
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-2031479036
should I try a rebase or is it useless ?
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-2218706152
Wow, we really fell off the wagon on this one. @squalou if you want to rebase, I'm optimistic we can get this through. CI has stabilized quite a bit lately, and it seems the conflicts might not be too crazy.
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on code in PR #27229:
URL: https://github.com/apache/superset/pull/27229#discussion_r1517331762
##########
superset/utils/excel.py:
##########
@@ -19,16 +19,36 @@
import pandas as pd
+from superset.utils.core import GenericDataType
-def df_to_excel(df: pd.DataFrame, **kwargs: Any) -> Any:
+
+def df_to_excel(
+ df: pd.DataFrame, coltypes: list[GenericDataType], **kwargs: Any
+) -> Any:
output = io.BytesIO()
# timezones are not supported
for column in df.select_dtypes(include=["datetimetz"]).columns:
df[column] = df[column].astype(str)
+ ndf = convert_df_with_datatypes(df, coltypes)
+
# pylint: disable=abstract-class-instantiated
with pd.ExcelWriter(output, engine="xlsxwriter") as writer:
- df.to_excel(writer, **kwargs)
+ ndf.to_excel(writer, **kwargs)
return output.getvalue()
+
+
+def convert_df_with_datatypes(
Review Comment:
As en end user, I much more prefer having an xlsx file with data I have to manually reformat, than having nothing at all.
> Catch and convert to a default. We also need to alert the user that some values were modified when exporting.
probably same as "dates with timezone" fix : `df[column].astype(str)`
Alerting the user is faaaaaaaar beyond anything I would be able to do, and agani as an end user I think I would'nt care. Nothing is said about timezone conversion either and I bet nobody cares.
>Don't catch and show a good message clearly indicating the column/row that is preventing the export.
As said, as an end user I would rather have a not-perfectly formatted file, but will all data in it, than no data at all.
- About "untouched fields" :
I don't really remember all the details (because it dates a bit and it's not my passion tbh :) ), but at the time, I manually opened the produced xlsx files, meaning : unzip it and see the xml-ish data inside and try to understand the differences when converted data or not. From there, open in libreoffice or MS and see the behaviors, and try to find the best.
Numeric values are stored "in the cells", while others have references in the cell and values mapped somewhere else. Or maybe it's the other way'round - can't even remember which is which. Anyway there's a clear separation between numeric and 'other'. Then, inside the 'other' category of course there may be further differences.
From my understanding then, the best thing one can do to expect any office software to deal with these files is to make sure numerics are numerics, anything doesn't matter that much and autodetection of strings work.
That's probably why `df[column] = df[column].astype(str)` in the date conversion has no real bad effect on usability.
That's all I know, which is not much for such a long text :)
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on code in PR #27229:
URL: https://github.com/apache/superset/pull/27229#discussion_r1516556751
##########
superset/utils/excel.py:
##########
@@ -19,16 +19,36 @@
import pandas as pd
+from superset.utils.core import GenericDataType
-def df_to_excel(df: pd.DataFrame, **kwargs: Any) -> Any:
+
+def df_to_excel(
+ df: pd.DataFrame, coltypes: list[GenericDataType], **kwargs: Any
+) -> Any:
output = io.BytesIO()
# timezones are not supported
for column in df.select_dtypes(include=["datetimetz"]).columns:
df[column] = df[column].astype(str)
+ ndf = convert_df_with_datatypes(df, coltypes)
+
# pylint: disable=abstract-class-instantiated
with pd.ExcelWriter(output, engine="xlsxwriter") as writer:
- df.to_excel(writer, **kwargs)
+ ndf.to_excel(writer, **kwargs)
return output.getvalue()
+
+
+def convert_df_with_datatypes(
Review Comment:
Addendum : a quick test regarding the "else" conversion, it fails with this error
```
TypeError: Cannot interpret '<GenericDataType.STRING: 1>' as a data type
```
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on code in PR #27229:
URL: https://github.com/apache/superset/pull/27229#discussion_r1516332226
##########
superset/utils/excel.py:
##########
@@ -19,16 +19,36 @@
import pandas as pd
+from superset.utils.core import GenericDataType
-def df_to_excel(df: pd.DataFrame, **kwargs: Any) -> Any:
+
+def df_to_excel(
+ df: pd.DataFrame, coltypes: list[GenericDataType], **kwargs: Any
+) -> Any:
output = io.BytesIO()
# timezones are not supported
for column in df.select_dtypes(include=["datetimetz"]).columns:
df[column] = df[column].astype(str)
+ ndf = copy_df_with_datatype_conversion(df, coltypes)
Review Comment:
I almost forgot the most important part : I have absolutely no idea how to know what dtypes are 'stored' in the DataFrame at the beginning :) so I used the 'coltypes' that seemed to be tehre for a reason :)
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-1980242374
(damn, import sort was not set, fixed and re-pushed)
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "squalou (via GitHub)" <gi...@apache.org>.
squalou commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-1967407707
> > Any help / pointer to where to begin with would be appreciated.
>
> You can create a test in [tests/unit_tests/common](https://github.com/apache/superset/blob/master/tests/unit_tests/common) similarly to [test_get_aggregated_join_column.py](https://github.com/apache/superset/blob/master/tests/unit_tests/common/test_get_aggregated_join_column.py)
Thank you ! will have a look asap (on vacation break this weak :D )
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #27229:
URL: https://github.com/apache/superset/pull/27229#issuecomment-2000197024
Hah... I'm rerunning the failing test now in hopes that it's just flaky. Fingers crossed.
--
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: set columns numeric datatypes when exporting to excel [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #27229: fix: set columns numeric datatypes when exporting to excel
URL: https://github.com/apache/superset/pull/27229
--
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