You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@parquet.apache.org by Deepak Majeti <ma...@gmail.com> on 2019/05/29 06:46:44 UTC
[DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Hi Everyone,
In the early days of parquet-cpp development, the developers mapped the
thrift::ConvertedType to parquet::LogicalType.
This now leads to confusion with the recent introduction of
thrift::LogicalType.
Parquet Rust also adopted this incorrect naming convention.
Are there any objections to renaming the Parquet::LogicalType to
Parquet::ConvertedType in both C++ and Rust?
--
regards,
Deepak Majeti
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Posted by Joris Van den Bossche <jo...@gmail.com>.
Op wo 29 mei 2019 om 10:00 schreef Antoine Pitrou <an...@python.org>:
>
> Why "converted"? Is there a conversion?
>
> "Converted" is the terminology used in the parquet format:
https://github.com/apache/parquet-format/blob/b5d34faf47b59b1220a1bbe0fc438be71fed6d90/src/main/thrift/parquet.thrift#L43-L48
>
> Le 29/05/2019 à 08:46, Deepak Majeti a écrit :
> > Hi Everyone,
> >
> > In the early days of parquet-cpp development, the developers mapped the
> > thrift::ConvertedType to parquet::LogicalType.
> > This now leads to confusion with the recent introduction of
> > thrift::LogicalType.
> >
> > Parquet Rust also adopted this incorrect naming convention.
> >
> > Are there any objections to renaming the Parquet::LogicalType to
> > Parquet::ConvertedType in both C++ and Rust?
> >
>
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Posted by Francois Saint-Jacques <fs...@gmail.com>.
+1 on renaming for to avoid confusion at the cost of breaking some API.
On Wed, May 29, 2019 at 10: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 <an...@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.
> > > >
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Posted by 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.
> > > > > >
> > >
>
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Posted by 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.
> > > > > >
> > >
>
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Posted by 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.
> > > > >
> >
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Posted by 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.
> > > > >
> >
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Posted by Chao Sun <su...@apache.org>.
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.
> > > >
>
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Posted by Chao Sun <su...@apache.org>.
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.
> > > >
>
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Posted by Wes McKinney <we...@gmail.com>.
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 <an...@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.
> > >
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Posted by Wes McKinney <we...@gmail.com>.
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 <an...@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.
> > >
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Posted by Wes McKinney <we...@gmail.com>.
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 <an...@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.
> >
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Posted by Wes McKinney <we...@gmail.com>.
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 <an...@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.
> >
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Posted by Joris Van den Bossche <jo...@gmail.com>.
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 <an...@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.
>
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to
Parquet::ConvertedType
Posted by Antoine Pitrou <an...@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.
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to Parquet::ConvertedType
Posted by Deepak Majeti <ma...@gmail.com>.
"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
On Wed, May 29, 2019 at 1:30 PM Antoine Pitrou <an...@python.org> wrote:
>
> Why "converted"? Is there a conversion?
>
>
> Le 29/05/2019 à 08:46, Deepak Majeti a écrit :
> > Hi Everyone,
> >
> > In the early days of parquet-cpp development, the developers mapped the
> > thrift::ConvertedType to parquet::LogicalType.
> > This now leads to confusion with the recent introduction of
> > thrift::LogicalType.
> >
> > Parquet Rust also adopted this incorrect naming convention.
> >
> > Are there any objections to renaming the Parquet::LogicalType to
> > Parquet::ConvertedType in both C++ and Rust?
> >
>
--
regards,
Deepak Majeti
Re: [DISCUSS] Parquet C++/Rust: Rename Parquet::LogicalType to
Parquet::ConvertedType
Posted by Antoine Pitrou <an...@python.org>.
Why "converted"? Is there a conversion?
Le 29/05/2019 à 08:46, Deepak Majeti a écrit :
> Hi Everyone,
>
> In the early days of parquet-cpp development, the developers mapped the
> thrift::ConvertedType to parquet::LogicalType.
> This now leads to confusion with the recent introduction of
> thrift::LogicalType.
>
> Parquet Rust also adopted this incorrect naming convention.
>
> Are there any objections to renaming the Parquet::LogicalType to
> Parquet::ConvertedType in both C++ and Rust?
>