PARQUET-481: Refactored and expanded reader-test and scanner-test.

Changes:

Moved Scanner tests from reader-test.cc to scanner-test.cc
Expanded both tests to run on multiple files
Added ParquetFileReader.JsonPrint() method to print a file contents in JSON format
Added several parquet files and corresponding json files for testing (these files will be removed once we have write functionality).

I haven't reviewed this yet carefully, but I'm -1 on this testing approach (test cases that rely on pre-generated data files and output) at this stage of the library's development. Can you explain why we should be doing this while the library is so immature and poorly unit tested?
I would be supportive of setting up an external integration test repository where we can put things like this to assist with smoke-testing the library while we're building up our unit test capabilities.

Both reader and scanner need their corresponding tests. While it's true that the library does not yet have all features implemented, it is not a reason to not have tests for the components that are implemented. The library has many interacting components, and it is important to confirm that we maintain their full integration and can actually read parquet files correctly. As we add support for more features (encodings, compression, etc.), they can be reflected in these tests, too, in addition to the corresponding unit tests for those features.
Obviously, once we can write parquet, we'll get rid of the pre-generated files.

I believe we should be doing automated integration testing with parquet-mr when the time comes, but I don't believe ad hoc integration tests have a place in parquet-cpp's git history -- by all means we should have an external smoke test repo. So I see a few options:

Assist with implementing both read and write support and unit test tooling to verify each functional component in isolation sufficient to be satisfied with its functionality
Set up an external smoke test repo to place a growing collection of files we want to be sure we can scan correctly
Work on automated integration testing with parquet-mr

@nongli @julienledem can you chime in with your thoughts here. thanks

Perhaps, I am missing something here, but (aside from a philosophical discussion on the value of end-to-end tests) what is wrong with adding tests that ensures that we can read/scan a file correctly? They are both unit tests for ParquetFileReader and Scanner, as well as integration tests for the underlying components. Without integration test, when someone modifies a component that changes its interaction with another component in a way that results in an incorrectly read file, this change will slip through because unit tests do not check for this. We can catch it later, when we integrate with parquet-mr or can write parquet, but why postpone catching and fixing an error if we can do now?

Technically speaking they are not unit tests, they are UI tests on static data. I'm opposed to approaching new software in this manner while the unit tests remain inadequate.
I'm supportive of having such tests, but not stored in this repo. In addition to bloating the git repo size (not as much of an issue with SVN / CVS), they tend to be brittle, especially with new software that is developing quickly. In my experience, in newer software they also create a false sense of security.
Integration testing will help us find and fix flaws in the implementation, but it should be automated and not based on static data. When integration testing finds a bug, making the integration test pass is generally not acceptable by itself except in exceptional cases -- you need to write a unit test that isolates and reproduces the root cause of the integration test failure to resolve it.
Like I said, happy to have an external repo for this and help keep its tests passing as long as maintaining them doesn't become onerous for me.

Hi,
I think we all agree that every component need their corresponding tests.
In general I agree with the points @wesm makes about checking in pre-generated files:
I think we should not check in binary files that were generated somewhere else in the unit tests. This makes it hard to change anything to the test or understand what it is doing.
These are brittle because they are too coarse. Which also makes them harder to debug when they fail.
This, in the end impedes productivity. When more focused unit tests pinpoint exactly the part that's broken and tend to be more stable since they fail only when that part fails.
I also understand that @asandryh's priority is getting a Parquet reader working. So we need to enable that too. But i'd think we can unit test the various bits of a reader without depending on the writer.
It sounds like we need to decide where to put such integration tests. I can ask a new git repo for this but in the meantime that should not be a blocker either. Anyone can create a github repo and get started and others can submit PRs to it too.
Also, because of the way git works checking in and updating binary files grows the repo and becomes a burden over time. Which is another argument for a repo dedicated to this.
Does the separate repo sound like the way to go?

+1. This has the side benefit of validating (on a continuous basis) that libparquet can be used as a 3rd-party library.

Let me look into setting up a separate repo with either pre-generated files or parquet-mr integration. I will also check if and how this can be added to Travis CI.

Cool -- we should not hook this into parquet-cpp's CI process, though, as it will slow down builds -- when an integration test fails we can open a new JIRA to reproduce the bug and add a unit test to fix.

I meant CI for the testing repo. Basically, I'd like every commit to parquet-cpp to trigger the reading tests. I don't know if this can be done automatically, though.

I see, that would be nice. I have nightly Jenkins builds for other projects I work on, hopefully Travis CI at least supports that use case.

Here's what I've found: At this time, Travis supports neither triggers for third-party repos nor nightly builds. We could add a custom script to parquet-cpp/.travis.yml -- but I don't think it's worth it in our case, since we expect to have write-read tests fairly soon. Let's revisit this discussion in a couple of weeks.

In the meantime, you could run a nightly cron job that pushes a commit to the integration test repo to trigger a Travis CI build. Not the most robust solution but it should suffice for a few months at least.
We can put a badge in the README so it's easy to check the build status similar to:
https://github.com/pydata/pandas/blob/master/README.md

Thanks, I'll look into this!
I will shortly update this PR to not include json file.

Please remove the new .parquet files too. Can you add some unit tests for the JSON code (check out the new test fixture tools used in parquet/column/column-reader-test)? JSON is a can of worms because reconstructing nested JSON is somewhat onerous (and unclear how useful, versus yielding in-memory data structures).

Another question: do you intend for JsonPrint to be used for anything other than debugging / integration test purposes? If not, it may be good to move both JsonPrint and DebugPrint to a separate parquet::debug namespace and compilation unit. I'd rather move them out of the ParquetFileReader public API

I am going to remove JsonPrint, since it's purpose was only to compare with json files. As for DebugPrint: it badly needs a re-write, and yes, it does not belong to ParquetFileReader. I will not do it in this PR, but rather file a separate issue to add parquet_cpp::test::ParquetFilePrinter class that prints a file contents out.

Can you let me know before you embark on that DebugPrint rewrite? Things are going to be a bit chaotic while I'm working on PARQUET-442 (should be complete by the end of the week if not sooner).

Sure, it's not urgent. I'll cc you on the jira, and will work on it once I get a green light from you.

@wesm @julienledem Please review the updated PR and comment.

I still have some mixed feelings about this patch. The first chance I get (within the next 4-7 days), I plan to change all these tests to use the MockPageReader test fixture and remove the test data files from the repository entirely. This is part of why I've been working tirelessly to make these components easier to unit test. I would rather do that directly rather than refactor only to refactor again.
It is nice to have an integration test helper class, I will review now.
I will let either @nongli and @julienledem pass final judgment with fair warning that either way everything related to the data/ directory has a limited half life in this repository.

On review I am -0 and will defer to the others. I would rather replace the existing code with unit tests and delete the test data files.

It looks like PARQUET-497 will introduce a lot of changes to the testing of ParquetFileReader, including changes made in this patch. To avoid modifying the code twice, I'm going to remove this PR for now and keep the changes on my branch.
@wesm Thank you for the comments, I'll address them on my branch.


Not working in Solaris 10: tail command

nvm.sh fails due to differences in tail implementation. I'm getting the usage: tail [+/-[n][lbc][f]] [file] error and it seems the answer here (http://stackoverflow.com/questions/314321/whats-the-solaris-equivalent-to-the-bsds-tail-n100) might be used to solve the issue

(linking back to https://github.com/creationix/nvm/issues/1121#issuecomment-230773679)

How would I convert usages like tail -n +2?

I believe tail +2 should work, cannot check it right now

Sadly it doesn't seem to work outside of Solaris :-/ https://travis-ci.org/ljharb/nvm/jobs/142899811#L597

K, I think for the -n +n cases I can just use sed instead.


Update json-rpc architecture link

This link obviously exists, but clicking it currently leads to ERR_TOO_MANY_REDIRECTS. This might not be the right solution but I think it fixes the link
Link in question is here: https://dev.rootstock.io/rsk/node/architecture/

Hi @tippenein Thanks for submitting a PR.
The problem has been fixed by #974


Mast: Watch pcmalwaretips.com

Mast requests the watch of the watch_keyword pcmalwaretips\.com. See the MS search here and the Stack Exchange search in text, in URLs, and in code.

pcmalwaretips\.com has been seen in 1 true positive, 0 false positives, and 0 NAAs.

Approved by Daniil in Charcoal HQ