fix(python): Throw exception if dataframe is too large to be compatible with Excel

[banflam]

fixes #20870

I have what I believe is a working solution and it is passing all the pre-existing tests. However, I don't know how to test whether my change is creating the expected behavior or not (when the dataframe is too large to be compatible with Excel, an exception should be thrown).

This is my very first commit to an open-source project, so I apologize in advance for any mistakes I might have made and I look forward to hearing any and all feedback.

[codecov]

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.90%. Comparing base (7bf42dd) to head (394811d).
Report is 236 commits behind head on main.

Files with missing lines	Patch %	Lines
py-polars/polars/dataframe/frame.py	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #20900      +/-   ##
==========================================
+ Coverage   79.76%   79.90%   +0.14%     
==========================================
  Files        1569     1597      +28     
  Lines      222895   229120    +6225     
  Branches     2573     2619      +46     
==========================================
+ Hits       177782   183078    +5296     
- Misses      44521    45443     +922     
- Partials      592      599       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alexander-beedie]

Thanks for taking the time to contribute!
A couple of notes:

1. You need to account for the headers, which will also write a row, not just the data (so we will write +1 rows when include_headers is True).
2. The table can start from a cell other than A1, so you also need to account for a starting offset (see the position parameter).
3. While we're at it we should probably also check the max width too; Excel's max there is 16384 columns 🤔

[banflam]

Thank you for your feedback and guidance. I've made the changes.

With respect to the second point, it appears that position can be either a tuple representing the numerical offset from the first Excel cell, or a string representing the starting cell directly. I have accounted for the tuple but I'm not entirely sure how to parse the string to get an offset. Please let me know what to do.

[alexander-beedie]

I have accounted for the tuple but I'm not entirely sure how to parse the string to get an offset. Please let me know what to do.

The table_start variable holds this offset (zero-indexed), so you can use that; it will have already done the string conversion for you (if position was provided that way) 👍

[banflam]

Thanks for that. I've removed the old code and replaced it with code that depends on the table_finish variable instead

[alexander-beedie]

Good stuff; the new check looks great - let's just tweak the error message when the check fails (done) and add a unit test, then this looks good to go 😎👍

You can add a simple unit test in py-polars/tests/unit/io/test_spreadsheet.py; try writing a few rows at an extreme offset (say, a 10 row dataset at position "A1048570") to confirm that the expected error is generated (in that file you can see other examples using with pytest.raises that check specific error conditions).

[banflam]

Thanks for the feedback. I've also written the test you mentioned. I hope that this pull request is now fine to be merged

[banflam]

Hi, I'm just following up on this

[alexander-beedie]

Just updated the temporary path in the test (a fixed /tmp/ would fail on Windows, for example, so we have a "tmp_path" fixture that provides something suitable 👍)

[alexander-beedie]

Congratulations (and thanks) on your first PR 😎