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

Support error type for formula cell #399

Merged
merged 2 commits into from
Aug 11, 2021
Merged

Conversation

EnverOsmanov
Copy link
Collaborator

@EnverOsmanov EnverOsmanov commented Aug 10, 2021

When a cell has formula inside - LibreOffice shows it empty, but spark-excel silently skips(!) these rows.

@waiyan1612 could you explain, please, why PlainNumberReadSuite expects to skip rows? See failed test cases.

@waiyan1612
Copy link
Contributor

Hi @EnverOsmanov, when I was developing the test suite, I just followed the existing behaviour of the inferSchema. That's why in the suite, using inferSchema=true is expecting 4 rows but inferSchema=false is expecting 5 rows.

Perhaps @nightscape can provide a better context. We will also need to consider the backward compatibility of this change.

@nightscape
Copy link
Collaborator

nightscape commented Aug 11, 2021

The "correct" solution will land with #389 where one can specify a ParseMode which determines how to handle rows with corrupt data.
@quanghgx is actively working on that branch.
I'd be ok with merging this as a temporary solution after the tests are adapted accordingly.

@EnverOsmanov
Copy link
Collaborator Author

@waiyan1612 , thanks for update!

The test was adjusted to not skip rows.

Regarding backward compatibility - I believe skipping rows is a bug and shouldn't be an expected behavior. So it should be fine "break" such behavior.

@nightscape
Copy link
Collaborator

nightscape commented Aug 11, 2021

@EnverOsmanov LGTM! Feel free to squash merge 👍

@EnverOsmanov EnverOsmanov merged commit cc6a64c into main Aug 11, 2021
@EnverOsmanov EnverOsmanov deleted the feature/process-error-cells branch August 11, 2021 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants