Add `sheet_index` param in datafile API to handle excel files with multiple sheets #3195

IamEzio · 2023-09-01T05:43:48Z

Fixes part of #3027

This PR does the following:

Adds code to accept sheet_index parameter which is the sheet number in an excel file.
Updates excel importing code to handle cases when header is made false (i.e, use default column names).

Screenshots

sheet-name.mp4

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the develop branch of the repository
My commit messages follow best practices.
My code follows the established code style of the repository.
I added tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no
visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

IamEzio · 2023-09-01T06:29:45Z

@dmos62 PTAL. Thanks!

Anish9901 · 2023-09-01T12:44:37Z

mathesar/imports/excel.py

+    try:
+        dataframe = remove_empty_rows_and_columns_from_dataframe(
+            pandas.read_excel(data_file.file.path, data_file.sheet_name, header=header_row)
+        )
+    except ValueError:
+        # If sheet number has been passed as a parameter instead of sheet name.
+        dataframe = remove_empty_rows_and_columns_from_dataframe(
+            pandas.read_excel(data_file.file.path, int(data_file.sheet_name), header=header_row)
+        )


I am a bit skeptical about this, I think we should only use the index of the file so that our django model could stay consistent, this could be achieved by searching the name of the excel sheet in the list of all the sheet_names and then finding its index. However, finding the list of sheet_names could cause a lot of overhead using the ExcelFile.sheet_names or df.keys() as they require to load the whole excel file in memory instead of just the specified sheet as done here. I did however find a method to efficiently extract sheet names but idk if its worth it. I'd like to know your thoughts on this @dmos62.

I notice there's some confusion between index and name here. Sometimes the sheet_name is cast to an int, which I presume implies that it acts as an index. sheet_index and sheet_name should be distinct and if we'd want to use some kind of polymorphic variable that's sometimes a name and sometimes an index, we'd probably name it sheet_id (since both a name and an index are identifiers), but I don't like the idea of a variable sometimes being one thing and sometimes something else.

For what it's worth, I think an index identifier has a better simplicity/usefulness relation than a name identifier. We could support both, but keeping it minimal is desirable right now, I think.

The SO answer @Anish9901 linked to mentioned that loading excel files gets exponentially slower with size (in Pandas). Supposing that's true, we'd do well to fix that at some point. Maybe now. If not, we should definitely put a TODO comment with a link to that SO thread explaining what the problem is and what the solution could be (where the relevant code is found).

For what it's worth, I think an index identifier has a better simplicity/usefulness relation than a name identifier. We could support both, but keeping it minimal is desirable right now, I think.

Makes sense. I'll update the attribute to sheet_index instead of having a polymorphic variable.

Anish9901 · 2023-09-01T12:44:55Z

mathesar/tests/api/test_table_api.py

@@ -242,6 +242,7 @@ def _create_table(client, data_files, table_name, schema, import_target_table, d

    response = client.post('/api/db/v0/tables/', body)
    response_table = response.json()
+    print('\n\n', response_table, '\n\n')


You missed a print statement.

Thanks @Anish9901, removed it now

mathesar/utils/datafiles.py

…to sheet-number

IamEzio · 2023-09-06T04:18:13Z

@dmos62 @Anish9901 I have updated the PR. PTAL. Thanks!

dmos62 · 2023-09-06T11:58:43Z

mathesar/utils/datafiles.py

@@ -114,18 +114,17 @@ def create_datafile(data):
        )
    else:
        max_level = data.get('max_level', 0)
-        sheet_name = data.get('sheet_name', '1')
+        sheet_index = data.get('sheet_index', 1)


Why have default be 1 if the index is 0-indexed? This also conflicts with the analogous field in the DataFile django model whose default is 0. Is this an oversight?

I was reducing the sheet_index value received from the API just a line below, so I thought that if the POST request doesn't have this value, we can assign it default value 1 and then reduce it.

On second thought, I agree that the above method is confusing. I have two solutions:

Keep the value 0-indexed everywhere and inform users to give sheet index based on 0-indexing.

Keep the value 1-indexed everywhere -- when asking users and when assigning default value (which would be 1). We'll just reduce it when we call pandas.read_excel().

@dmos62 what are your thoughts?

I think 0-indexing is standard and we should go with that.

@dmos62 I have updated it now. PTAL. Thanks!

added code for accepting sheet name

2463b63

IamEzio marked this pull request as draft September 1, 2023 05:43

nit

6a4bc71

IamEzio marked this pull request as ready for review September 1, 2023 06:29

IamEzio requested a review from dmos62 September 1, 2023 06:29

IamEzio added this to the GSoC 2023 milestone Sep 1, 2023

IamEzio added the work: backend Related to Python, Django, and simple SQL label Sep 1, 2023

IamEzio mentioned this pull request Sep 1, 2023

Support for importing Excel files #3027

Closed

Anish9901 reviewed Sep 1, 2023

View reviewed changes

Anish9901 mentioned this pull request Sep 1, 2023

Meeting notes for GSoC JSON and Excel import project #3028

Closed

dmos62 requested changes Sep 4, 2023

View reviewed changes

mathesar/utils/datafiles.py Show resolved Hide resolved

dmos62 and others added 3 commits September 5, 2023 17:26

Merge branch 'develop' into sheet-number

8fc19f2

review fix

0e75f39

Merge branch 'sheet-number' of https://github.com/IamEzio/mathesar in…

6ca72b8

…to sheet-number

IamEzio changed the title ~~Add sheet_name param in datafile API to handle excel files with multiple sheets~~ Add sheet_index param in datafile API to handle excel files with multiple sheets Sep 6, 2023

dmos62 requested changes Sep 6, 2023

View reviewed changes

standardized 0-indexing

6071597

dmos62 approved these changes Sep 8, 2023

View reviewed changes

dmos62 added this pull request to the merge queue Sep 8, 2023

Merged via the queue into mathesar-foundation:develop with commit e7b175b Sep 8, 2023
9 checks passed

IamEzio deleted the sheet-number branch September 11, 2023 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `sheet_index` param in datafile API to handle excel files with multiple sheets #3195

Add `sheet_index` param in datafile API to handle excel files with multiple sheets #3195

IamEzio commented Sep 1, 2023 •

edited

IamEzio commented Sep 1, 2023

Anish9901 Sep 1, 2023

dmos62 Sep 4, 2023

IamEzio Sep 4, 2023 •

edited

Anish9901 Sep 1, 2023

IamEzio Sep 6, 2023

IamEzio commented Sep 6, 2023

dmos62 Sep 6, 2023 •

edited

IamEzio Sep 6, 2023

dmos62 Sep 7, 2023

IamEzio Sep 7, 2023

Add sheet_index param in datafile API to handle excel files with multiple sheets #3195

Add sheet_index param in datafile API to handle excel files with multiple sheets #3195

Conversation

IamEzio commented Sep 1, 2023 • edited

Checklist

Developer Certificate of Origin

IamEzio commented Sep 1, 2023

Anish9901 Sep 1, 2023

Choose a reason for hiding this comment

dmos62 Sep 4, 2023

Choose a reason for hiding this comment

IamEzio Sep 4, 2023 • edited

Choose a reason for hiding this comment

Anish9901 Sep 1, 2023

Choose a reason for hiding this comment

IamEzio Sep 6, 2023

Choose a reason for hiding this comment

IamEzio commented Sep 6, 2023

dmos62 Sep 6, 2023 • edited

Choose a reason for hiding this comment

IamEzio Sep 6, 2023

Choose a reason for hiding this comment

dmos62 Sep 7, 2023

Choose a reason for hiding this comment

IamEzio Sep 7, 2023

Choose a reason for hiding this comment

Add `sheet_index` param in datafile API to handle excel files with multiple sheets #3195

Add `sheet_index` param in datafile API to handle excel files with multiple sheets #3195

IamEzio commented Sep 1, 2023 •

edited

IamEzio Sep 4, 2023 •

edited

dmos62 Sep 6, 2023 •

edited