Skip to content

Conversation

@wesm
Copy link
Member

@wesm wesm commented Aug 24, 2017

This is mostly moving code around. In reviewing I recommend focusing on the public headers. There were a number of places where it is more consistent to use naked pointers versus shared_ptr. Also some constructors were returning shared_ptr to subclass, where it would be simpler for clients to return a pointer to base.

This includes ARROW-1376 and ARROW-1406

wesm added 7 commits August 24, 2017 15:02
Change-Id: Ic152224d842b7c19079487d3ba1ca531e51e0687
… from metadata.h to metadata-internal.h and create message.h, dictionary.h

Change-Id: I3fbacc6a0f5d02b734a04b9717d426cb1b89ace2
…anywhere

Change-Id: I3ba12a8874426b33c17b9d5491ef54965cce1051
Change-Id: If8f66ebb6d3914d8aa1936be0410079ab735de89
Change-Id: I6be0f1d53df9661a158e80e5b1a901336a46fae6
Change-Id: I031b65db10357b04cfcfc0b85162b4a51406efc5
Change-Id: Icccce78b5fc73c66088dbafcdf8a098a009371aa
@wesm
Copy link
Member Author

wesm commented Aug 24, 2017

Summary of deprecations:

  • Deprecated in favor of an InputStream based version (since the only difference is a Seek)
ARROW_EXPORT
Status ReadRecordBatch(const std::shared_ptr<Schema>& schema, int64_t offset,
                       io::RandomAccessFile* stream, std::shared_ptr<RecordBatch>* out);
  • RecordBatchStreamWriter and file writer return std::shared_ptr<RecordBatchWriter> since they have identical APIs

@cpcloud
Copy link
Contributor

cpcloud commented Aug 24, 2017

+1 LGTM. The API looks much more unified this way.

@cpcloud
Copy link
Contributor

cpcloud commented Aug 24, 2017

Looks like a transient conda error on appveyor:

CondaHTTPError: HTTP 502 BAD GATEWAY for url <https://repo.continuum.io/pkgs/pro/win-64/repodata.json.bz2>

@wesm
Copy link
Member Author

wesm commented Aug 25, 2017

Alright, merging

@asfgit asfgit closed this in f50f2ea Aug 25, 2017
@wesm wesm deleted the ARROW-1408 branch August 25, 2017 03:10
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.

2 participants