PARQUET-1701: [C++] Stream API: Add support for optional fields #5928
Conversation
|
will try to take a look at this within a week if nobody gets to it sooner. |
StreamReader and StreamWriter classes can now support schemata with optional fields. Additional input/output operators are available using the arrow::util::optional class in order to indicate whether optional data was present when reading, or is present when writing.
gawain-bolton
force-pushed
the
PARQUET-1701_stream_api_optional_fields
branch
from
4fe9e47
to
1efa7ff
Compare
| /// | ||
| class PARQUET_EXPORT StreamReader { | ||
| public: | ||
| template <typename T> | ||
| using optional = arrow::util::optional<T>; |
There was a problem hiding this comment.
sorry I thought I commented (and perhaps I missed the response) but I'm kind of against this alias.
There was a problem hiding this comment.
On the contrary this alias is useful to users of this class so that they can use the required optional type.
There was a problem hiding this comment.
I see the marginal convenience of it, I could go either way. I'm fine with leaving it
|
Seems reasonable except for the documentation and 1 style (using an alias for optional). So I think a committer should take a look @wesm @xhochy @majetideepak |
optional fields. Add tests to ensure that required fields can be read using operator>>(arrow::util::optional<T>). Add tests to ensure that required fields can be written using operator<<(arrow::util::optional<T>).
|
Will look. Are the CI failures legitimate? |
|
The macOS failure is probably https://issues.apache.org/jira/browse/ARROW-7551 (I say clicking on it) |
|
The appveyor failure looks unrelated, trying to rebuild. |
There was a problem hiding this comment.
It seems like there's parts of the tests that could be simplified through type parameterization and TEST_P. I'm not sure that changing everything to EXPECT from ASSERT is a good idea since there may be cases where one of the objects is left in an invalid state once one of the calls fails. I don't see anything (aside potentially for CI issues) to stop this from being merged, further cleaning / improvement could be done in a follow up patch. @gawain-bolton do you plan to make any other changes?
| /// | ||
| class PARQUET_EXPORT StreamReader { | ||
| public: | ||
| template <typename T> | ||
| using optional = arrow::util::optional<T>; |
There was a problem hiding this comment.
I see the marginal convenience of it, I could go either way. I'm fine with leaving it
|
@wesm : I do not plan on making any other changes to this pull request unless required. All the functionality required to support optional fields is in place and I would very much like this functionality to be available in the next release of Arrow. |
|
thanks @gawain-bolton! |
StreamReader and StreamWriter classes can now support schemata with optional fields. Additional input/output operators are available using the arrow::util::optional class in order to indicate whether optional data was present when reading, or is present when writing. Closes apache#5928 from gawain-bolton/PARQUET-1701_stream_api_optional_fields and squashes the following commits: e9d8001 <gawain.bolton> Rename tests to conform with codebase. 70d9b3b <gawain.bolton> Improve documentation on input/output operators for required and optional fields. 1efa7ff <gawain.bolton> Updates after code review by emkornfield on 20191229. de82604 <gawain.bolton> Eliminate warning on Visual Studio 2017 build 70233da <gawain.bolton> PARQUET-1701: Stream API: Add support for optional fields Authored-by: gawain.bolton <gawain.bolton@cfm.fr> Signed-off-by: Wes McKinney <wesm+git@apache.org>
StreamReader and StreamWriter classes can now support schemata with
optional fields.
Additional input/output operators are available using the
arrow::util::optional class in order to indicate whether optional data
was present when reading, or is present when writing.