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 <sunc...@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 <wesmck...@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 <wesmck...@gmail.com> wrote: > > > > > > You all probably want to join d...@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 > > > <jorisvandenboss...@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 <anto...@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. > > > > > > >
