Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType

[Joris Van den Bossche <jo...@gmail.com>]

I opened a JIRA for this: https://issues.apache.org/jira/browse/PARQUET-1603

Op vr 31 mei 2019 om 22:21 schreef Wes McKinney <we...@gmail.com>:

> It seems we have an in-flight Parquet C++ patch  (ARROW-3729) that
> touches the data types code -- I think we could do the grand search
> and replace after that
>
> On Wed, May 29, 2019 at 4:31 PM Chao Sun <su...@apache.org> wrote:
> >
> > I'm +1 on the change for the Rust side as well. It probably won't be as
> > disruptive as the C++ side.
> >
> > On Wed, May 29, 2019 at 7:09 AM Wes McKinney <we...@gmail.com>
> wrote:
> >
> > > I'm in favor of making the change -- it's slightly disruptive for
> > > library-users, but the fix is no more complicated than a
> > > search-and-replace. When the C++ project was started, the LogicalType
> > > union didn't exist and "LogicalType" seemed like a more appropriate
> > > name for ConvertedType.
> > >
> > > On Wed, May 29, 2019 at 7:11 AM Wes McKinney <we...@gmail.com>
> wrote:
> > > >
> > > > You all probably want to join dev@parquet.apache.org and have the
> > > > discussion there. From a governance perspective that's where we need
> > > > to talk about making breaking changes to the Parquet C++ library
> > > >
> > > > LogicalType was introduced into the Parquet format in October 2017 to
> > > > be a more flexible and future-proof replacement for the original
> > > > ConvertedType metadata, see
> > > >
> > > >
> > >
> https://github.com/apache/parquet-format/commit/863875e0be3237c6aa4ed71733d54c91a51deabe
> > > >
> > > > Support and forward/backwards compatibility for the new LogicalType
> > > > union was just developed in PARQUET-1411
> > > >
> > > >
> > >
> https://github.com/apache/arrow/commit/38b1ddfb7f5def825ac57c8f27ffe5afa7fcb483
> > > >
> > > > On Wed, May 29, 2019 at 4:44 AM Joris Van den Bossche
> > > > <jo...@gmail.com> wrote:
> > > > >
> > > > > Yes, the LogicalType is newer than ConvertedType in the parquet
> > > format, and
> > > > > was until recently not implemented in parquet-cpp.
> > > > > The problem is that originally, the parquet thrift::ConvertedType
> was
> > > > > implemented in parquet-cpp as LogicalType. Now, support is added in
> > > > > parquet-cpp for this newer parquet thrift::LogicalType, but the
> obvious
> > > > > name for that in parquet-cpp was already taken. Therefore, it was
> > > added as
> > > > > parquet::LogicalAnnotation. See this PR for context:
> > > > > https://github.com/apache/arrow/pull/4185
> > > > >
> > > > > So Deepak's question is if we can rename parquet-cpp's
> > > parquet::LogicalType
> > > > > to parquet::ConvertedType (to match the thrift format naming), so
> we
> > > can
> > > > > actually use the logical name parquet::LogicalType instead of
> > > > > parquet::LogicalAnnotation for the new implementation.
> > > > > And to avoid the confusion we are having here ..
> > > > >
> > > > > But renaming like that would be hard break in parquet-cpp for
> libraries
> > > > > depending on that, though. But I can't really assess the impact of
> > > that.
> > > > >
> > > > > Best,
> > > > > Joris
> > > > >
> > > > > Op wo 29 mei 2019 om 11:04 schreef Antoine Pitrou <
> antoine@python.org
> > > >:
> > > > >
> > > > > >
> > > > > > Le 29/05/2019 à 10:47, Deepak Majeti a écrit :
> > > > > > > "ConvertedType" term is used by the parquet specification
> below.
> > > This
> > > > > > type
> > > > > > > is used to map client data types to Parquet types.
> > > > > > >
> > > > > >
> > >
> https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift#L48
> > > > > >
> > > > > > But apparently there's also "LogicalType":
> > > > > >
> > > > > >
> > >
> https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift#L315
> > > > > >
> > > > > > "LogicalType annotations to replace ConvertedType"
> > > > > >
> > > > > > Regards
> > > > > >
> > > > > > Antoine.
> > > > > >
> > >
>