PARQUET-516: Better error handling in LocalFileSource#56
PARQUET-516: Better error handling in LocalFileSource#56asandryh wants to merge 2 commits intoapache:masterfrom
Conversation
| SerializedFile::~SerializedFile() { | ||
| Close(); | ||
| } | ||
|
|
There was a problem hiding this comment.
This addresses PARQUET-537
|
@wesm Can you look at this PR and let me know whether this is what you had in mind for PARQUET-516? Thanks. |
|
Yes, but we need to write unit tests to verify the correct functioning of these classes and their methods. |
asandryh
force-pushed
the
PARQUET-516
branch
from
80edb21 to
52f7f11
Compare
|
@wesm Right, will do. Just wanted to check first whether you had anything else in mind. |
|
In the tests the main thing will be failing in the expected ways (e.g. exceptions raised instead of core dump / segfault later). |
|
I'm working on a patch for PARQUET-520 and will add unit tests for the rest of input.h/cc. |
asandryh
force-pushed
the
PARQUET-516
branch
from
52f7f11 to
f1e9617
Compare
…7: Ensure LocalFileSource is closed properly.
asandryh
force-pushed
the
PARQUET-516
branch
from
f1e9617 to
3b488eb
Compare
|
@wesm I've updated this PR with unit tests. Please take a look. |
| ss << "Cannot seek to position " << pos << " in file [" << path_ | ||
| << "] of size " << size_; | ||
| throw ParquetException(ss.str()); | ||
| } |
There was a problem hiding this comment.
Some implementations of fseek() will not produce an error if you try to seek past the end of the file. Hence, we should track this explicitly.
|
Argh, we have crossed streams. |
|
Can you create a separate patch for PARQUET-537? I will incorporate the additional unit tests here into my patch-in-progress (which also includes a MemoryMapSource). |
|
Please see #66 and let me know if I missed any test cases here |
2 participants
Also, a fix for PARQUET-537: Ensure LocalFileSource is closed properly.