Skip to content
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

fix: Load tables when flag IReader::READ_DATA_ONLY is used #3726

Merged

Conversation

kevin-verschaeve
Copy link
Contributor

@kevin-verschaeve kevin-verschaeve commented Sep 12, 2023

Abstract

This PR adds the loading of the named tables when using the IReader::READ_DATA_ONLY flag

This is:

  • a bugfix
  • a new feature
  • refactoring
  • additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • Code style is respected
  • Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
  • Documentation is updated as necessary

Why this change is needed?

When loading a spreadsheet, using the flag IReader::READ_DATA_ONLY to be faster and use less memory, I encountered an issue.

The CalculationEngine tried to resolve a cell formula. That formula was refering to a named table. Named table are not loaded because of the IReader::READ_DATA_ONLY flag.

I feel that, when using that flag, we should either:

  • not try to resolve formulas at all
  • or: fully resolve them, which means loading named tables

For information, we did not notice any performance degradation when loading the named table vs not doing it, at least of the large spreadsheet files we are using.

@oleibman
Copy link
Collaborator

I think you are correct that Tables should be read even with dataOnly. Can you please add a unit test that demonstrates that your change works as desired?

@kevin-verschaeve
Copy link
Contributor Author

cool !
sure, I'll try to do it next week.

Thanks

@kevin-verschaeve kevin-verschaeve force-pushed the fix/read-data-only-include-tables branch from 15e12ca to 6f5b9bc Compare September 13, 2023 09:28
@kevin-verschaeve
Copy link
Contributor Author

I've added a test, please tell me if it seems good for you

@kevin-verschaeve kevin-verschaeve force-pushed the fix/read-data-only-include-tables branch 2 times, most recently from 2a7b0bb to 7be5814 Compare September 13, 2023 10:26
@oleibman
Copy link
Collaborator

Looks good. I will try to merge this in a day or two.

@kevin-verschaeve kevin-verschaeve force-pushed the fix/read-data-only-include-tables branch from 7be5814 to 7b30b43 Compare September 14, 2023 07:44
@oleibman oleibman merged commit 6fbf474 into PHPOffice:master Sep 14, 2023
@oleibman
Copy link
Collaborator

Thank you for your contribution.

@kevin-verschaeve kevin-verschaeve deleted the fix/read-data-only-include-tables branch September 14, 2023 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants