Re: PARQUET-1411 / PR 4185

[Wes McKinney <we...@gmail.com>]

+ Parquet dev list

Thanks Tim for working on this issue, I'm sorry I haven't been able to
leave code review yet -- I've been busy with a bunch of other things
and, since it's a large patch, I wanted to give thoughtful feedback.

Feel free to push some more commits to that PR. I can prioritize
getting you some feedback in the next couple of working days, just let
me know when you're ready for me to review.

On Thu, May 2, 2019 at 5:23 PM TP Boudreau <tp...@gmail.com> wrote:
>
> Hello Parquet-Arrow Team, Wes,
>
> A short while ago, I submitted PR 4185 (
> https://github.com/apache/arrow/pull/4185) to implement in the C++ library
> the new logical annotations metadata available in the latest parquet.thrift
> spec (https://issues.apache.org/jira/browse/PARQUET-1411).  I stopped
> committing to that PR's branch about a week ago to allow the code to be
> reviewed without it being a moving target.
>
> I've since (optimistically) starting blocking out new code for ARROW-3729
> based on my open PR (switching Arrow to read/write the new Parquet
> annotations, https://issues.apache.org/jira/browse/ARROW-3729) and while
> doing that realized that usage of the annotations classes I created in the
> open PR might be smoother with the introduction of a few convenience
> methods.  However, the most suitable names for these methods (IMO) were
> introduced for another purpose in the open PR and would need to be
> reclaimed -- overall a fairly minor, non-structural change to the PR code.
>
> I can either add another commit to the open PR to add these convenience
> methods and rename some things (which would be my preference, provided no
> one has invested too much time yet reviewing it -- maybe you have Wes?), or
> I can wait for the first round of reviews on that PR to see where things
> stand.
>
> How should I proceed?
>
> Thanks in advance,
> --Tim

Re: PARQUET-1411 / PR 4185

[TP Boudreau <tp...@gmail.com>]

No need for apologies, Wes, I appreciate you keeping this on your radar.

I've made the changes and have pushed them to the PR branch.  You can begin
your review when you get the chance.

--TPB

On Thu, May 2, 2019 at 3:32 PM Wes McKinney <we...@gmail.com> wrote:

> + Parquet dev list
>
> Thanks Tim for working on this issue, I'm sorry I haven't been able to
> leave code review yet -- I've been busy with a bunch of other things
> and, since it's a large patch, I wanted to give thoughtful feedback.
>
> Feel free to push some more commits to that PR. I can prioritize
> getting you some feedback in the next couple of working days, just let
> me know when you're ready for me to review.
>
> On Thu, May 2, 2019 at 5:23 PM TP Boudreau <tp...@gmail.com> wrote:
> >
> > Hello Parquet-Arrow Team, Wes,
> >
> > A short while ago, I submitted PR 4185 (
> > https://github.com/apache/arrow/pull/4185) to implement in the C++
> library
> > the new logical annotations metadata available in the latest
> parquet.thrift
> > spec (https://issues.apache.org/jira/browse/PARQUET-1411).  I stopped
> > committing to that PR's branch about a week ago to allow the code to be
> > reviewed without it being a moving target.
> >
> > I've since (optimistically) starting blocking out new code for ARROW-3729
> > based on my open PR (switching Arrow to read/write the new Parquet
> > annotations, https://issues.apache.org/jira/browse/ARROW-3729) and while
> > doing that realized that usage of the annotations classes I created in
> the
> > open PR might be smoother with the introduction of a few convenience
> > methods.  However, the most suitable names for these methods (IMO) were
> > introduced for another purpose in the open PR and would need to be
> > reclaimed -- overall a fairly minor, non-structural change to the PR
> code.
> >
> > I can either add another commit to the open PR to add these convenience
> > methods and rename some things (which would be my preference, provided no
> > one has invested too much time yet reviewing it -- maybe you have Wes?),
> or
> > I can wait for the first round of reviews on that PR to see where things
> > stand.
> >
> > How should I proceed?
> >
> > Thanks in advance,
> > --Tim
>