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

No error message displayed on CSV load exception #2606

Closed
philrz opened this issue Dec 5, 2022 · 1 comment · Fixed by #2651
Closed

No error message displayed on CSV load exception #2606

philrz opened this issue Dec 5, 2022 · 1 comment · Fixed by #2651
Assignees
Labels
bug Something isn't working community

Comments

@philrz
Copy link
Contributor

philrz commented Dec 5, 2022

Repro is with Brim commit 96a0951 which uses Zed commit 22f640e. This issue feels very similar to the existing JSON-related one in #2554.

A community zync user reported a problem reading CSV with Zui. In their own words:

Zui did not report any problem. It just stopped the CSV import without any message at all. I noticed because I compared count() to the wc.
maybe simply send a more comprehensive error message in Zed/Zui when CSV import fails before the EOF? line number and column number with last value parsed?

Indeed, as shown in the following video, reading the file bad.csv using CLI tooling does show an error, but the app just shows the data that parsed ok without any mention of the error. This is a data correctness hazard since it gives the user the impression they're working with all their data when it's actually truncated.

Repro.mp4

As we can see in Wireshark, the error message was returned from the backend, but just didn't make it all the way to the user.

image

@philrz philrz added bug Something isn't working community labels Dec 5, 2022
@nwt nwt self-assigned this Jan 17, 2023
@nwt nwt closed this as completed in #2651 Feb 7, 2023
@philrz
Copy link
Contributor Author

philrz commented Feb 7, 2023

Verified in Zui commit a372438.

As shown in the attached video, now the user is shown a warning when this partial load occurs, then they can click this for further detail.

Verify.mp4

It's been pointed out that the "Load successful" pop-up does represent a contradictory message alongside the "Ingest failed with warnings" message, so #2660 has been opened to improve on that.

Thanks @nwt!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working community
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants