Skip to content

ENH: Add calamite engine to read_excel #50581

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 48 commits into from
Closed
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
30da9a4
ENH: add calamite excel reader and modify test to include engine
kostyafarber Jan 5, 2023
a47d3fb
Merge branch 'main' into issue-50395
kostyafarber Jan 5, 2023
6c1dd87
Merge branch 'main' into issue-50395
kostyafarber Jan 5, 2023
fd06ad9
fix deps for python-calamine
kostyafarber Jan 5, 2023
8b6200a
Merge branch 'main' into issue-50395
kostyafarber Jan 5, 2023
6a8d822
fix deps for python-calamine, add as pip package
kostyafarber Jan 5, 2023
efcb2fc
ENH: fix typo in engine declaration, add import_optional_dependency, …
kostyafarber Jan 7, 2023
e1105de
Merge branch 'main' into issue-50395
kostyafarber Jan 11, 2023
6b50e0c
calamite -> calamine, updated some tests for calamine
dimastbk Jan 11, 2023
0784733
calamine excel engine: skip tests with datetime
dimastbk Jan 12, 2023
5971199
Merge branch 'main' into issue-50395
kostyafarber Jan 12, 2023
655318b
Merge branch 'issue-50395' into issue-50395-dima
dimastbk Jan 18, 2023
cc049cf
Merge branch 'main' into issue-50395
kostyafarber Jan 20, 2023
038133e
ENH: change reader filename match library, fix typo in engine name in…
kostyafarber Jan 22, 2023
52c2cbd
Merge branch 'issue-50395' into issue-50395-dima
kostyafarber Jan 22, 2023
2dc5e02
Merge pull request #1 from dimastbk/issue-50395-dima
kostyafarber Jan 22, 2023
2076e11
Merge pull request #2 from dimastbk/issue-50395-dima-skip-tests
kostyafarber Jan 22, 2023
6b0a7ac
Merge branch 'main' into issue-50395
kostyafarber Jan 22, 2023
a614089
ENH: add back get_sheet_by_index
kostyafarber Jan 22, 2023
256f9f9
Merge branch 'main' into issue-50395
kostyafarber Jan 23, 2023
eee8b4e
Merge branch 'main' into issue-50395
kostyafarber Jan 23, 2023
9fc2209
ENH: fix mypy and trailing whitespace
kostyafarber Jan 24, 2023
cf1268a
Merge branch 'main' into issue-50395
kostyafarber Jan 24, 2023
bebfec5
Merge branch 'main' into issue-50395
kostyafarber Jan 24, 2023
9019904
added conversion date/time/float, support file_rows_needed, fixed sup…
dimastbk Jan 26, 2023
08a5616
Update test_readers.py
dimastbk Jan 26, 2023
677a224
Merge pull request #3 from dimastbk/issue-50395
kostyafarber Jan 26, 2023
8c55e5d
Merge branch 'main' into issue-50395
kostyafarber Jan 26, 2023
d817999
update python-calamine to 0.0.7
dimastbk Jan 27, 2023
12aaf19
Merge branch 'main' into issue-50395
kostyafarber Jan 28, 2023
255e8fb
Merge pull request #4 from dimastbk/issue-50395
kostyafarber Jan 28, 2023
500fa9f
Merge branch 'main' into issue-50395
kostyafarber Jan 29, 2023
5d94728
fix review: use CalamineReader/CalamineSheet
dimastbk Jan 31, 2023
15874c3
fixed pyright, fixed docs in __init__
dimastbk Feb 6, 2023
89ae49e
Merge pull request #5 from dimastbk/issue-50395
kostyafarber Feb 7, 2023
33e5b7e
Merge branch 'main' into issue-50395
kostyafarber Feb 7, 2023
85d31ec
Merge branch 'main' into issue-50395
kostyafarber Mar 23, 2023
a0d4193
Merge branch 'main' into issue-50395
kostyafarber Mar 25, 2023
a6b6fb2
bump python-calamine to 0.1.0
dimastbk Mar 26, 2023
0a431c5
_ValueT -> _CellValueT
dimastbk Mar 29, 2023
745cd09
Merge pull request #6 from dimastbk/issue-50395
kostyafarber Mar 29, 2023
942a16a
Merge branch 'main' into issue-50395
kostyafarber Apr 1, 2023
8803ca9
Merge branch 'main' into issue-50395
kostyafarber Apr 2, 2023
2f5ffba
added xfail to tests, small fixes
dimastbk Apr 3, 2023
b8b1a9a
Merge pull request #7 from dimastbk/issue-50395
kostyafarber Apr 8, 2023
f5ab40d
Merge branch 'main' into issue-50395
kostyafarber Apr 8, 2023
02c2e7f
bump calamine to 0.1.1, update tests (472 passed, 75 xfailed), update…
dimastbk May 1, 2023
74a3e70
Merge pull request #8 from dimastbk/issue-50395
kostyafarber May 1, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ci/deps/actions-310.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,6 @@ dependencies:
- xlrd
- xlsxwriter
- zstandard

- pip:
- python-calamine
3 changes: 3 additions & 0 deletions ci/deps/actions-38-downstream_compat.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,6 @@ dependencies:
- pandas-gbq
- pyyaml
- py

- pip:
- python-calamine
1 change: 1 addition & 0 deletions ci/deps/actions-38-minimum_versions.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,4 @@ dependencies:

- pip:
- pyqt5==5.15.1
- python-calamine==0.0.7
3 changes: 3 additions & 0 deletions ci/deps/actions-38.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,6 @@ dependencies:
- xlrd
- xlsxwriter
- zstandard

- pip:
- python-calamine
3 changes: 3 additions & 0 deletions ci/deps/actions-39.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,6 @@ dependencies:
- xlrd
- xlsxwriter
- zstandard

- pip:
- python-calamine
3 changes: 3 additions & 0 deletions ci/deps/circle-38-arm64.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,6 @@ dependencies:
- xlrd
- xlsxwriter
- zstandard

- pip:
- python-calamine
4 changes: 3 additions & 1 deletion doc/source/whatsnew/v2.0.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,11 @@ Other enhancements
- :meth:`Series.drop_duplicates` has gained ``ignore_index`` keyword to reset index (:issue:`48304`)
- Improved error message in :func:`to_datetime` for non-ISO8601 formats, informing users about the position of the first error (:issue:`50361`)
- Improved error message when trying to align :class:`DataFrame` objects (for example, in :func:`DataFrame.compare`) to clarify that "identically labelled" refers to both index and columns (:issue:`50083`)
- Performance improvement in :func:`to_datetime` when format is given or can be inferred (:issue:`50465`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are in the processing of cutting 2.0.0 now; at this point should target 2.1.0 for this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kostyafarber, hi! Can you merge main in issue-50395?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep will do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dimastbk merged!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is still in the v2.0.0 whatsnew

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I asked for merging main in pr branch. Now fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should revert any changes to this file - the v2.0.0.rst file shouldn't be touched as part of this PR

- Added ``calamine`` as an engine to ``read_excel`` (:issue: ``50395``)
- Added :meth:`DatetimeIndex.as_unit` and :meth:`TimedeltaIndex.as_unit` to convert to different resolutions; supported resolutions are "s", "ms", "us", and "ns" (:issue:`50616`)
- Added new argument ``dtype`` to :func:`read_sql` to be consistent with :func:`read_sql_query` (:issue:`50797`)
-


.. ---------------------------------------------------------------------------
.. _whatsnew_200.notable_bug_fixes:
Expand Down
1 change: 1 addition & 0 deletions environment.yml
Original file line number Diff line number Diff line change
Expand Up @@ -117,3 +117,4 @@ dependencies:

- pip:
- sphinx-toggleprompt
- python-calamine
2 changes: 2 additions & 0 deletions pandas/compat/_optional.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"pyarrow": "6.0.0",
"pyreadstat": "1.1.2",
"pytest": "7.0.0",
"python-calamine": "0.0.7",
"pyxlsb": "1.0.8",
"s3fs": "2021.08.0",
"scipy": "1.7.1",
Expand Down Expand Up @@ -61,6 +62,7 @@
"lxml.etree": "lxml",
"odf": "odfpy",
"pandas_gbq": "pandas-gbq",
"python_calamine": "python-calamine",
"snappy": "python-snappy",
"sqlalchemy": "SQLAlchemy",
"tables": "pytables",
Expand Down
2 changes: 2 additions & 0 deletions pandas/io/excel/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -1450,6 +1450,7 @@ class ExcelFile:
This is not supported, switch to using ``openpyxl`` instead.
"""

from pandas.io.excel._calaminereader import CalamineExcelReader
from pandas.io.excel._odfreader import ODFReader
from pandas.io.excel._openpyxl import OpenpyxlReader
from pandas.io.excel._pyxlsb import PyxlsbReader
Expand All @@ -1460,6 +1461,7 @@ class ExcelFile:
"openpyxl": OpenpyxlReader,
"odf": ODFReader,
"pyxlsb": PyxlsbReader,
"calamine": CalamineExcelReader,
}

def __init__(
Expand Down
110 changes: 110 additions & 0 deletions pandas/io/excel/_calaminereader.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
from __future__ import annotations

from datetime import (
date,
datetime,
time,
)
from tempfile import NamedTemporaryFile
from typing import Union

from pandas._typing import (
FilePath,
ReadBuffer,
Scalar,
StorageOptions,
)
from pandas.compat._optional import import_optional_dependency

import pandas as pd

from pandas.io.common import stringify_path
from pandas.io.excel._base import (
BaseExcelReader,
inspect_excel_format,
)

ValueT = Union[int, float, str, bool, time, date, datetime]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we stick with the same Scalar import that the other readers use?

Copy link
Contributor

@dimastbk dimastbk Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type is for internal using in _convert_cell. get_sheet_data returns Scalar.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this into the function it is actually used in? Sounds like it has a pretty localized use, so no need to be in the global namespace

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I see in https://peps.python.org/pep-0613/#scope-restrictions, typealiases can't defined within function scope. But, I can make ValueT is protected in module level.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah fair point. At the very least I think a more descriptive name than ValueT would be better. What is this supposed to represent? Some type of cell scalar?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed in _CellValueT.



class __calamine__:
pass


class CalamineExcelReader(BaseExcelReader):
book: str
_sheet_names: list[str] | None = None

def __init__(
self,
filepath_or_buffer: FilePath | ReadBuffer[bytes],
storage_options: StorageOptions = None,
) -> None:
import_optional_dependency("python_calamine")
super().__init__(filepath_or_buffer, storage_options=storage_options)

@property
def _workbook_class(self) -> type[__calamine__]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the idea of typing but I don't think this is a correct type, especially since it will be inconsistent with other reader classes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

return __calamine__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea here that this doesn't need a real value to fulfill the requirements of the abstract class?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


def load_workbook(self, filepath_or_buffer) -> str:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just seems to return a file name whereas the other readers return a physical workbook object. Does calamine not have a similar concept?

Copy link
Contributor

@dimastbk dimastbk Jan 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, python-calamine doesn't support returning data as workbook. You can use https://github.com/ToucanToco/fastexcel, if you need this feature.

I can add a wrapper for result of get_sheet_data.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I think we should follow the requirements of the ABC here - otherwise things get pretty wonky across the various implementations. How hard would it be to implement that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made simple implementation of these classes (kostyafarber#5), but I need some time to test it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WillAyd I rewrote this code to use classes.

if hasattr(filepath_or_buffer, "read") and hasattr(filepath_or_buffer, "seek"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can get rid of all of this if _workbook_class is properly implemented; looks like the base class should handle things gracefully

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

ext = inspect_excel_format(filepath_or_buffer)
with NamedTemporaryFile(suffix=f".{ext}", delete=False) as tmp_file:
filepath_or_buffer.seek(0)
tmp_file.write(filepath_or_buffer.read())
filepath_or_buffer = tmp_file.name
else:
filepath_or_buffer = stringify_path(filepath_or_buffer)

assert isinstance(filepath_or_buffer, str)

from python_calamine import get_sheet_names

self._sheet_names = get_sheet_names(filepath_or_buffer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be set here - seems duplicate of the code in sheet_names as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put this code in load_workbook to check file by calamine as early as possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment as above - we should trust the ABC to do the work and just implement the requirements it already has set forth

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

return filepath_or_buffer

@property
def sheet_names(self) -> list[str]:
from python_calamine import get_sheet_names

if self._sheet_names is None:
self._sheet_names = get_sheet_names(self.book)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to accomplish this without assigning to a new attribute?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

return self._sheet_names

def get_sheet_by_name(self, name: str) -> int:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is supposed to return a sheet not an int

self.raise_if_bad_sheet_by_name(name)
return self.sheet_names.index(name)

def get_sheet_by_index(self, index: int) -> int:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here as above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

self.raise_if_bad_sheet_by_index(index)
return index

def get_sheet_data(
self, sheet: int, file_rows_needed: int | None = None
) -> list[list[Scalar]]:
def _convert_cell(value: ValueT) -> Scalar:
if isinstance(value, float):
val = int(value)
if val == value:
return val
else:
return value
elif isinstance(value, date):
return pd.Timestamp(value)
elif isinstance(value, time):
return value.isoformat()

return value

from python_calamine import get_sheet_data

rows = get_sheet_data(self.book, sheet, skip_empty_area=False)
data: list[list[Scalar]] = []

for row in rows:
data.append([_convert_cell(cell) for cell in row])
if file_rows_needed is not None and len(data) >= file_rows_needed:
break

return data
Loading