Skip to content
This repository was archived by the owner on May 10, 2024. It is now read-only.

PARQUET-520: Add MemoryMapSource and add unit tests for both it and LocalFileSource#66

Closed
wesm wants to merge 4 commits intoapache:masterfrom
wesm:PARQUET-520
Closed

PARQUET-520: Add MemoryMapSource and add unit tests for both it and LocalFileSource#66
wesm wants to merge 4 commits intoapache:masterfrom
wesm:PARQUET-520

Conversation

@wesm
Copy link
Member

@wesm wesm commented Feb 29, 2016

I also added the file_descriptor API so that we can verify that dtors elsewhere successfully close open files. Closes #56

@wesm
Copy link
Member Author

wesm commented Feb 29, 2016

@asandryh let me know if there's anything additional from #56 that I can add here; we should prefer this patch as it runs the same set of tests both on memory-mapped / non-memory-mapped file readers. We need a test case reproduction of PARQUET-537 before fixing that bug.


ASSERT_THROW(this->source.Seek(this->filesize_ + 1), ParquetException);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also test Read() functionality. See an example in #56.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do

@wesm
Copy link
Member Author

wesm commented Feb 29, 2016

All set. let me know any more comments, thank you!

@asandryh
Copy link
Contributor

+1 LGTM.

@wesm
Copy link
Member Author

wesm commented Feb 29, 2016

@julienledem
Copy link
Member

+1

@asfgit asfgit closed this in 41c1e68 Mar 1, 2016
@wesm
Copy link
Member Author

wesm commented Mar 1, 2016

thank you!

@wesm wesm deleted the PARQUET-520 branch March 1, 2016 00:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants