feat: table support for Excel (XLSX) sheets #282

wdoppenberg · 2024-09-03T09:30:28Z

Implemented functionality to read and work with named tables within Excel sheets. Updated .gitignore, added corresponding tests.

One note: I'm using a clone() in the build_table method, please advise on how to get rid of this. Also, please give me feedback on eager vs. lazy loading, I believe some refinement might be necessary there.

Related issue: #204

Implemented functionality to read and work with named tables within Excel sheets, extending support for lazy loading and error handling. Updated `.gitignore`, added corresponding tests, and refined error structures and build processes.

lukapeschke · 2024-09-03T09:46:50Z

Oh wow thanks a lot! I'll review this ASAP. In the meantime, could you please rebase the branch and run make format ? Looks like a lot of code has been re-indented to 8 spaces, and we use 4

Implemented functionality to read and work with named tables within Excel sheets, extending support for lazy loading and error handling. Updated `.gitignore`, added corresponding tests, and refined error structures and build processes.

…ture/extract-tables # Conflicts: # python/fastexcel/_fastexcel.pyi # python/tests/test_tables.py # src/types/python/excelreader.rs # src/types/python/excelsheet/table.rs # test.py

lukapeschke

Thanks a lot for your work, looks great overall 🚀

Since it's a pretty big change, I'd like @PrettyWood to have a look as well, as I might have missed some parts (he'll be back from vacation next week).

Also, I wonder wether we should introduce a new type for tables: It'd allow us to store some extra metadata, and would allow us to add to_{arrow,polars,pandas} methods as well. As a nice side effect, it would also avoid the clone there currently is without needing changes on the calamine side

Anyway, this has motivated me again to polish the README and add some proper contributor docs 😅

Makefile

python/fastexcel/__init__.py

src/error.rs

lukapeschke · 2024-09-03T15:52:10Z

src/types/python/excelreader.rs

+            // TODO: Remove clone
+            ExcelSheetData::from(range.clone()),


Here, I believe the best solution would be to submit a change to calamine: Either make the data field pub in or add a pub into_data(self) -> T method to the Table type

I've opened a PR: tafia/calamine#464

Can you update the TODO with "to update once calamine v.... is added"

src/types/python/excelreader.rs

test.py

src/types/python/excelreader.rs

Co-authored-by: Luka Peschke <mail@lukapeschke.com>

dhcsousa · 2024-09-06T07:36:57Z

Very cool feature, it would also be useful for me!

PrettyWood

LGTM! Thanks a lot

PrettyWood · 2024-09-17T14:44:36Z

src/types/python/excelreader.rs

+            // TODO: Remove clone
+            ExcelSheetData::from(range.clone()),


Can you update the TODO with "to update once calamine v.... is added"

PrettyWood · 2024-09-17T14:52:02Z

Thanks a lot for your work, looks great overall 🚀

Since it's a pretty big change, I'd like @PrettyWood to have a look as well, as I might have missed some parts (he'll be back from vacation next week).

Also, I wonder wether we should introduce a new type for tables: It'd allow us to store some extra metadata, and would allow us to add to_{arrow,polars,pandas} methods as well. As a nice side effect, it would also avoid the clone there currently is without needing changes on the calamine side

Anyway, this has motivated me again to polish the README and add some proper contributor docs 😅

@lukapeschke Yup I reckon we should differentiate Sheet and Table. I don' know if you want to work on that @wdoppenberg directly in this PR. It would be great but if you don't have the time we can merge this PR like that and work on a refacto after that on our end

wdoppenberg · 2024-09-17T15:08:35Z

Thanks a lot for your work, looks great overall 🚀
Since it's a pretty big change, I'd like @PrettyWood to have a look as well, as I might have missed some parts (he'll be back from vacation next week).
Also, I wonder wether we should introduce a new type for tables: It'd allow us to store some extra metadata, and would allow us to add to_{arrow,polars,pandas} methods as well. As a nice side effect, it would also avoid the clone there currently is without needing changes on the calamine side
Anyway, this has motivated me again to polish the README and add some proper contributor docs 😅

@lukapeschke Yup I reckon we should differentiate Sheet and Table. I don' know if you want to work on that @wdoppenberg directly in this PR. It would be great but if you don't have the time we can merge this PR like that and work on a refacto after that on our end

I don't have time for this this week. So depending on your timelines you might want to merge and open a new branch. If you can wait a week I can spend some time then.

lukapeschke · 2024-09-17T15:40:16Z

@wdoppenberg Let's merge this as is then to avoid more merge conflicts, and I'll see what I can do regarding the new Table object 🙂 Could you please give me push rights on your fork ? I'll fix the conflicts and merge the PR afterwards

wdoppenberg · 2024-09-17T18:03:23Z

@wdoppenberg Let's merge this as is then to avoid more merge conflicts, and I'll see what I can do regarding the new Table object 🙂 Could you please give me push rights on your fork ? I'll fix the conflicts and merge the PR afterwards

Should work:

Signed-off-by: Luka Peschke <luka.peschke@toucantoco.com>

lukapeschke

LGTM! Thanks a lot @wdoppenberg 🙏

wdoppenberg added 2 commits September 3, 2024 11:22

Add table support for Excel sheets

50f7ca3

Implemented functionality to read and work with named tables within Excel sheets, extending support for lazy loading and error handling. Updated `.gitignore`, added corresponding tests, and refined error structures and build processes.

Add docstring to table_names method and modify load_table docstring

b742d99

lukapeschke self-requested a review September 3, 2024 09:42

wdoppenberg added 6 commits September 3, 2024 12:52

Add table support for Excel sheets

216e037

Implemented functionality to read and work with named tables within Excel sheets, extending support for lazy loading and error handling. Updated `.gitignore`, added corresponding tests, and refined error structures and build processes.

Add docstring to table_names method and modify load_table docstring

a2ae25a

Formatting

3a498cc

Merge remote-tracking branch 'origin/feature/extract-tables' into fea…

11c911e

…ture/extract-tables # Conflicts: # python/fastexcel/_fastexcel.pyi # python/tests/test_tables.py # src/types/python/excelreader.rs # src/types/python/excelsheet/table.rs # test.py

Fixed eager arg in load_table

f21e2ef

Remove unused XlsxError variant

3d66e36

wdoppenberg changed the title ~~Add table support for Excel (XLSX) sheets~~ feat: table support for Excel (XLSX) sheets Sep 3, 2024

wdoppenberg mentioned this pull request Sep 3, 2024

feat: named table data as DataRef type tafia/calamine#464

Merged

lukapeschke requested changes Sep 3, 2024

View reviewed changes

lukapeschke added enhancement New feature or request 🐍 python 🐍 Pull requests that edit Python code 🦀 rust 🦀 Pull requests that edit Rust code 🗒️ calamine 🗒️ Calamine-related issue labels Sep 3, 2024

lukapeschke requested a review from PrettyWood September 3, 2024 16:19

wdoppenberg and others added 8 commits September 3, 2024 18:20

Update Makefile

5b5e407

Co-authored-by: Luka Peschke <mail@lukapeschke.com>

Update test.py

dbf819c

Co-authored-by: Luka Peschke <mail@lukapeschke.com>

Update src/types/python/excelreader.rs

4b90f84

Co-authored-by: Luka Peschke <mail@lukapeschke.com>

Update method signatures and header row handling

ea12f81

Remove redundant XlsxError exception

0cb0054

formatting

1964c2e

Updated header_row arg docs for load_table

4cafbfb

Refine table printing logic

96f74a1

wdoppenberg requested a review from lukapeschke September 3, 2024 18:55

lukapeschke added the ✋ need review ✋ label Sep 11, 2024

PrettyWood approved these changes Sep 17, 2024

View reviewed changes

Adjusted TODO stmt

150718e

lukapeschke added 2 commits September 18, 2024 09:52

Merge remote-tracking branch 'toucan/main' into feature/extract-tables

d89cc59

docs: added comment about sheet metadata

cad7745

Signed-off-by: Luka Peschke <luka.peschke@toucantoco.com>

lukapeschke approved these changes Sep 18, 2024

View reviewed changes

lukapeschke merged commit 79a7f17 into ToucanToco:main Sep 18, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: table support for Excel (XLSX) sheets #282

feat: table support for Excel (XLSX) sheets #282

wdoppenberg commented Sep 3, 2024 •

edited

Loading

lukapeschke commented Sep 3, 2024

lukapeschke left a comment

lukapeschke Sep 3, 2024

wdoppenberg Sep 4, 2024

PrettyWood Sep 17, 2024

dhcsousa commented Sep 6, 2024

PrettyWood left a comment

PrettyWood Sep 17, 2024

PrettyWood commented Sep 17, 2024

wdoppenberg commented Sep 17, 2024

lukapeschke commented Sep 17, 2024

wdoppenberg commented Sep 17, 2024

lukapeschke left a comment

feat: table support for Excel (XLSX) sheets #282

feat: table support for Excel (XLSX) sheets #282

Conversation

wdoppenberg commented Sep 3, 2024 • edited Loading

lukapeschke commented Sep 3, 2024

lukapeschke left a comment

Choose a reason for hiding this comment

lukapeschke Sep 3, 2024

Choose a reason for hiding this comment

wdoppenberg Sep 4, 2024

Choose a reason for hiding this comment

PrettyWood Sep 17, 2024

Choose a reason for hiding this comment

dhcsousa commented Sep 6, 2024

PrettyWood left a comment

Choose a reason for hiding this comment

PrettyWood Sep 17, 2024

Choose a reason for hiding this comment

PrettyWood commented Sep 17, 2024

wdoppenberg commented Sep 17, 2024

lukapeschke commented Sep 17, 2024

wdoppenberg commented Sep 17, 2024

lukapeschke left a comment

Choose a reason for hiding this comment

wdoppenberg commented Sep 3, 2024 •

edited

Loading