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

Added support for read_xport #50

Closed
wants to merge 1 commit into from

Conversation

cipherz
Copy link

@cipherz cipherz commented Aug 18, 2019

  • C_interface.jl:
    Changed return type for readstat_get_row_count to Int (from UInt), as it seems that not all format contains nrows as metainformation and -1 is returned in those instances (xport is one of such formats).
    Added readstat_parse for type::Val{:xport}, calls readstat_parse_xport.

  • ReadStat.jl:
    Added constant READSTAT_NALLOC_ROWS: number of rows to allocate at the same time (see handle_variable! and handle_value!)
    Added read_xport and exported it.
    Modified read_data_file, handle_variable! and handle_value! to support formats without nrows metainformation.

  • Test
    Added test cases for xpt and types.xpt.

 - C_interface.jl:
   Changed return type for readstat_get_row_count to Int (from UInt), as it seems that not all format contains nrows as metainformation and -1 is returned in those instances (xport is one of such formats).
   Added readstat_parse for type::Val{:xport}, calls readstat_parse_xport.

 - ReadStat.jl:
   Added constant READSTAT_NALLOC_ROWS: number of rows to allocate at the same time (see handle_variable! and handle_value!)
   Added read_xport and exported it.
   Modified read_data_file, handle_variable! and handle_value! to support formats without nrows metainformation.

- Test
  Added test cases for xpt and types.xpt.
@davidanthoff
Copy link
Member

Cool, thanks!

I was not aware that readstat_get_row_count can return -1, that seems important to handle, thanks for spotting that! I'm wondering, though, whether we should handle that case differently. I think in that case I would just allocate vectors with 0 elements, and then push! into those. I think the algorithm in push! is better at growing arrays than using a fixed increment like READSTAT_NALLOC_ROWS.

I'll try to fix the CI failures on master, and then you can merge master back into this branch and we'll hopefully get CI tests passing.

@davidanthoff
Copy link
Member

Alrigh, #51 fixed the CI builds on master. So if you merge master into your branch, you should have a baseline that passes CI builds.

@cipherz
Copy link
Author

cipherz commented Aug 25, 2019 via email

@davidanthoff
Copy link
Member

Yes, I think having a different code path for the case with known size and the case without would be good, we don't want to use push! if we know the size beforehand!

@vjd
Copy link

vjd commented Mar 20, 2021

Just following up here to see if there are plans to support xpt files?

@andreasnoack andreasnoack mentioned this pull request Mar 27, 2021
@davidanthoff
Copy link
Member

I believe this is fully covered by #75, so closing this here.

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