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

Update parquet parser #855

Closed
wants to merge 4 commits into from
Closed

Update parquet parser #855

wants to merge 4 commits into from

Conversation

Giqles
Copy link

@Giqles Giqles commented Apr 8, 2022

This is a fix for #854.

# insert reprex here

PR task list:

  • Update NEWS
  • Add tests
  • Update documentation with devtools::document()

@CLAassistant
Copy link

CLAassistant commented Apr 8, 2022

CLA assistant check
All committers have signed the CLA.

if (!requireNamespace("readr", quietly = TRUE)) {
stop("`readr` must be installed for `parser_parquet` to work")
}
readr::write_file(value, tmpfile)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What advantage is there to use readr::write_file() over writeBin()?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have an example of a rare issue?

Copy link
Author

Choose a reason for hiding this comment

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

I was trying to write a test to for the issue (I thought) I was having with the writeBin implementation. The original data I was having this issue (#854) with is not public, and some alternatives I've tried didn't seem to reproduce either.

I've just gone back to the machine I was trying this on, and it doesn't reproduce there either -- I think this must've been caused by something else. I'll close this PR for now.

@schloerke
Copy link
Collaborator

@Giqles Any other changes you'd like to do to make this PR not a draft?

@Giqles
Copy link
Author

Giqles commented Jun 2, 2022

Closing this for now. Does not reproduce consistently.

@Giqles Giqles closed this Jun 2, 2022
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