[GitHub] [incubator-superset] villebro commented on a change in pull request #10450: fix: excel sheet upload is not working

[GitBox <gi...@apache.org>]

villebro commented on a change in pull request #10450:
URL: https://github.com/apache/incubator-superset/pull/10450#discussion_r462065343



##########
File path: superset/views/database/forms.py
##########
@@ -371,6 +368,13 @@ def at_least_one_schema_is_allowed(database: Database) -> bool:
         validators=[Optional(), NumberRange(min=0)],
         widget=BS3TextFieldWidget(),
     )
+    parse_dates = CommaSeparatedListField(
+        _("Parse Dates"),
+        description=_(
+            "A comma separated list of columns that should be " "parsed as dates."

Review comment:
       nit: remove the redundant `" "`

##########
File path: superset/views/database/views.py
##########
@@ -307,16 +306,29 @@ def form_post(self, form: ExcelToDatabaseForm) -> Response:
             database = (
                 db.session.query(models.Database).filter_by(id=con.data.get("id")).one()
             )
+
+            # some params are not supported by pandas.read_excel (e.g. chunksize).
+            # More can be found here:
+            # https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.read_excel.html
             excel_to_df_kwargs = {
                 "header": form.header.data if form.header.data else 0,
                 "index_col": form.index_col.data,
                 "mangle_dupe_cols": form.mangle_dupe_cols.data,
-                "skipinitialspace": form.skipinitialspace.data,
                 "skiprows": form.skiprows.data,
                 "nrows": form.nrows.data,
-                "sheet_name": form.sheet_name.data,
-                "chunksize": 1000,
+                "sheet_name": (
+                    int(form.sheet_name.data)
+                    if form.sheet_name.data.isdigit()
+                    else form.sheet_name.data
+                )
+                if form.sheet_name.data
+                else 0,

Review comment:
       While I understand the convenience aspect of supporting both `str` and `int` here, this won't work if a sheet name is truly a digit. I'd prefer to standardize on a single approach, i.e. either only support integer values (=zero indexed position) or assume that any value is the sheet name. Personally I'd vote for the following option:
   - if no value is provided, assume it's the first sheet.
   - if a value is provided, treat it as a string. Entering the value "1" would look for a sheet with the name "1", not the second sheet.
   




----------------------------------------------------------------
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.

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