ENH: Add check for character limmit #57103

luke396 · 2024-01-27T08:07:57Z

closes ENH: show warning when exporting dataframe and hitting excel truncating limit #56954 (Replace xxxx with the GitHub issue number)
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added type annotations to new arguments/methods/functions.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

luke396 · 2024-01-27T08:13:30Z

As discussed in the context of #56954 (comment), we have incorporated a validation for the maximum row/column number within the code.

pandas/pandas/io/formats/excel.py

Lines 929 to 934 in 9008ee5

    
           num_rows, num_cols = self.df.shape 
        
           if num_rows > self.max_rows or num_cols > self.max_cols: 
        
               raise ValueError( 
        
                   f"This sheet is too large! Your sheet size is: {num_rows}, {num_cols} " 
        
                   f"Max sheet size is: {self.max_rows}, {self.max_cols}" 
        
               )

Furthermore, I have introduced a character limit check and included corresponding notifications in the documentation.

luke396 · 2024-01-27T13:41:33Z

The CI test failure appears to be linked to the changes made in the pull request, but I'm unable to reproduce it on my local machine (WSL2). Assistance may be needed.

luke396 · 2024-01-28T05:15:02Z

The CI test failure appears to be linked to the changes made in the pull request

Caused by warnings from other packages, and should be resolved by incorporating raise_on_extra_warnings=False

rhshadrach

Thanks for the PR, looks good! Some requests below, also needs a note in the 3.0.0 whatsnew in the Other Enhancements section.

pandas/io/excel/_base.py

rhshadrach · 2024-01-28T14:42:38Z

pandas/io/excel/_base.py

+            # GH#56954
+            if len(val) > 32767:
+                warnings.warn(
+                    "String value too long, truncated to 32767 characters",


This can potentially come from other data types, "String" may end up being confusing if that's the case. I think a more generic "Cell contents too long" is better.

I have a quick question regarding this: should we also explore validating other data types, like float or int? The current code seems to focus solely on validating the length for specific data types. Do we need to extend these checks to other types mentioned above? Alternatively, have we conducted similar checks elsewhere, or is there a best practice that should be adhered to in this context?

should we also explore validating other data types, like float or int?

I think Excel's limitation is specifically for strings.

Alternatively, have we conducted similar checks elsewhere, or is there a best practice that should be adhered to in this context?

When you say elsewhere, are you thinking of outside the excel code? I do not know of any formal best practice.

I believe I grasp your perspective. I want to ensure we deliberate on whether we should also account for the length of other data types. For now, let's concentrate on checking the length specifically for strings in this pull request; that should suffice.

pandas/core/generic.py

mroeschke · 2024-01-29T19:43:33Z

pandas/io/excel/_base.py

+            if len(val) > 32767:
+                raise ValueError(


Can you add a reference where 32767 comes from?

Also I think this should be a UserWarning and not a ValueError. We shouldn't completely stop writing the excel file due to excel's limitation

I had suggested an error over a warning in #57103 (comment) primarily due to the fact that we raise an error if the frame is too long or wide already.

Ah I see. The reason I suggested warning was due to a similar scenario I remember a while back where we used to raise an error if we detected whether a stack/unstack operation would create "too many elements" and users wanted to bypass that limitation. We ended up changing it to a PerformanceWarning #45084

I slightly prefer to still raise a UserWarning to make pandas less opinionated here (the Excel output is still valid and maybe a truncated value is OK for some users). But if you feel strongly about a ValueError, it's not a blocker for me

Actually - raising here would need a deprecation too, right? That makes me think we should raise a warning. @luke396 - can you turn this back into a warning?

Actually - raising here would need a deprecation too, right?

Conservatively yes, but it would "fix" an undesired (external) behavior ("bug").

Truth be told, I didn't initially utilize 'UserWarning' to the extent you both recommended when I first started out. However, I gained valuable insights from the discussions you provided.

doc/source/whatsnew/v3.0.0.rst

Co-authored-by: Matthew Roeschke <[email protected]>

mroeschke · 2024-01-31T18:41:07Z

pandas/tests/io/excel/test_writers.py

+        df = DataFrame({"A": ["a" * 32768]})
+        msg = "Cell contents too long, truncated to 32767 characters"
+        with tm.assert_produces_warning(
+            UserWarning, match=msg, raise_on_extra_warnings=False


What extra warning is raised here?

From commit babdf29,

the check https://github.com/pandas-dev/pandas/actions/runs/7678882560/job/20929191559 rise pandas/tests/io/excel/test_writers.py::TestExcelWriter::test_to_excel_raising_warning_when_cell_character_exceed_limit[openpyxl-.xlsx] - AssertionError: Caused unexpected warning(s): [('EncodingWarning', EncodingWarning("'encoding' argument not specified"), '/home/runner/micromamba/envs/test/lib/python3.10/site-packages/openpyxl/writer/excel.py', 199)]

and check https://github.com/pandas-dev/pandas/actions/runs/7678882560/job/20929192274 rasie pandas/tests/io/excel/test_writers.py::TestExcelWriter::test_to_excel_raising_warning_when_cell_character_exceed_limit[openpyxl-.xlsm] - AssertionError: Caused unexpected warning(s): [('DeprecationWarning', DeprecationWarning('datetime.datetime.utcnow() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.datetime.now(datetime.UTC).'), '/home/runner/micromamba/envs/test/lib/python3.12/site-packages/openpyxl/packaging/core.py', 99), ('DeprecationWarning', DeprecationWarning('datetime.datetime.utcnow() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.datetime.now(datetime.UTC).'), '/home/runner/micromamba/envs/test/lib/python3.12/site-packages/openpyxl/writer/excel.py', 292)]

pandas/tests/io/excel/test_writers.py::TestExcelWriter::test_to_excel_raising_warning_when_cell_character_exceed_limit is what we added in this pr. I just wrote the test following the other's in same file. If there is a better way to avoid these extra warnings, I am very happy to change codes!

Gotcha. Yeah since these are external warnings this is fine for now

mroeschke

Merge when ready @rhshadrach

rhshadrach

lgtm

rhshadrach · 2024-02-01T21:37:12Z

Thanks @luke396!

Co-authored-by: Matthew Roeschke <[email protected]>

Add check for character limmit

20e357a

luke396 requested a review from rhshadrach as a code owner January 27, 2024 08:07

luke396 added 4 commits January 27, 2024 16:18

Run pre-commit

74e302f

Fix test

74f4f81

Merge branch 'main' into add-character-check-to-excel-cell

d864912

Fix test

babdf29

Fix test ingnore extra warings

4768089

rhshadrach requested changes Jan 28, 2024

View reviewed changes

rhshadrach reviewed Jan 28, 2024

View reviewed changes

pandas/core/generic.py Outdated Show resolved Hide resolved

luke396 added 3 commits January 28, 2024 23:15

Improve by review

65284bb

Add what's new

41cad2b

Merge branch 'main' into add-character-check-to-excel-cell

5470464

mroeschke reviewed Jan 29, 2024

View reviewed changes

doc/source/whatsnew/v3.0.0.rst Outdated Show resolved Hide resolved

mroeschke added the IO Excel read_excel, to_excel label Jan 29, 2024

luke396 and others added 4 commits January 30, 2024 16:37

Update doc/source/whatsnew/v3.0.0.rst

2e79c61

Co-authored-by: Matthew Roeschke <[email protected]>

Merge branch 'main' into add-character-check-to-excel-cell

91a4bee

Improve

62b8b54

Change what's new

bd6a975

luke396 requested review from mroeschke and rhshadrach January 30, 2024 10:59

mroeschke reviewed Jan 31, 2024

View reviewed changes

mroeschke added this to the 3.0 milestone Feb 1, 2024

mroeschke approved these changes Feb 1, 2024

View reviewed changes

rhshadrach added Enhancement Warnings Warnings that appear or should be added to pandas labels Feb 1, 2024

rhshadrach approved these changes Feb 1, 2024

View reviewed changes

rhshadrach merged commit bb42fc0 into pandas-dev:main Feb 1, 2024

luke396 deleted the add-character-check-to-excel-cell branch February 2, 2024 02:27

pmhatre1 pushed a commit to pmhatre1/pandas-pmhatre1 that referenced this pull request May 7, 2024

ENH: Add check for character limit (pandas-dev#57103)

67bd76d

Co-authored-by: Matthew Roeschke <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Add check for character limmit #57103

ENH: Add check for character limmit #57103

luke396 commented Jan 27, 2024

luke396 commented Jan 27, 2024

luke396 commented Jan 27, 2024

luke396 commented Jan 28, 2024

rhshadrach left a comment

rhshadrach Jan 28, 2024

luke396 Jan 28, 2024

rhshadrach Jan 29, 2024

luke396 Jan 30, 2024

mroeschke Jan 29, 2024

rhshadrach Jan 29, 2024 •

edited

Loading

mroeschke Jan 29, 2024

rhshadrach Jan 29, 2024

mroeschke Jan 29, 2024

luke396 Jan 30, 2024

mroeschke Jan 31, 2024

luke396 Feb 1, 2024

mroeschke Feb 1, 2024

mroeschke left a comment

rhshadrach left a comment

rhshadrach commented Feb 1, 2024

ENH: Add check for character limmit #57103

ENH: Add check for character limmit #57103

Conversation

luke396 commented Jan 27, 2024

luke396 commented Jan 27, 2024

luke396 commented Jan 27, 2024

luke396 commented Jan 28, 2024

rhshadrach left a 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

Choose a reason for hiding this comment

rhshadrach Jan 29, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroeschke left a comment

Choose a reason for hiding this comment

rhshadrach left a comment

Choose a reason for hiding this comment

rhshadrach commented Feb 1, 2024

rhshadrach Jan 29, 2024 •

edited

Loading