PARQUET-2355: Deprecate `parquet-thrift` by Fokko · Pull Request #1175 · apache/parquet-java

Fokko · 2023-10-18T13:47:52Z

Make sure you have checked all steps below.

parquet-thrift relies on Elephantbird: https://github.com/twitter/elephant-bird

The current version of elephant-bird that we use is still on Thrift 0.7.0, and this doesn't work with 0.19.0 that we're trying to upgrade to. Updating is non-trivial since a lot of the code that was used for testing has been made private: #1156

I would suggest removing this from the repository since it looks like no one is using it anymore:

apache-thrift does not seem to be used anymore on Maven Central (usages of 0 since 1.12.2):

Jira

My PR addresses the following Parquet Jira issues and references them in the PR title. For example, "PARQUET-1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-2355
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does

Fokko · 2023-10-18T16:03:59Z

pom.xml

              <exclude>org.apache.parquet.hadoop.ColumnChunkPageWriteStore</exclude>
              <exclude>org.apache.parquet.hadoop.ParquetRecordWriter</exclude>
+              <!-- Already deprecated classes/methods/constants -->
+              <exclude>org.apache.parquet.hadoop.thrift.ThriftReadSupport#setProjectionPushdown(org.apache.hadoop.mapred.JobConf,java.lang.String)</exclude>


https://github.com/apache/parquet-mr/blob/5ee513341158bc3962df80260c345b80076dbf3e/parquet-thrift/src/main/java/org/apache/parquet/hadoop/thrift/ThriftReadSupport.java#L104-L107

Fokko · 2023-10-18T16:04:19Z

pom.xml

              <exclude>org.apache.parquet.hadoop.ParquetRecordWriter</exclude>
+              <!-- Already deprecated classes/methods/constants -->
+              <exclude>org.apache.parquet.hadoop.thrift.ThriftReadSupport#setProjectionPushdown(org.apache.hadoop.mapred.JobConf,java.lang.String)</exclude>
+              <exclude>org.apache.parquet.hadoop.thrift.ThriftReadSupport#THRIFT_COLUMN_FILTER_KEY</exclude>


https://github.com/apache/parquet-mr/blob/5ee513341158bc3962df80260c345b80076dbf3e/parquet-thrift/src/main/java/org/apache/parquet/hadoop/thrift/ThriftReadSupport.java#L52-L57

Fokko · 2023-10-18T16:04:39Z

pom.xml

+              <!-- Already deprecated classes/methods/constants -->
+              <exclude>org.apache.parquet.hadoop.thrift.ThriftReadSupport#setProjectionPushdown(org.apache.hadoop.mapred.JobConf,java.lang.String)</exclude>
+              <exclude>org.apache.parquet.hadoop.thrift.ThriftReadSupport#THRIFT_COLUMN_FILTER_KEY</exclude>
+              <exclude>org.apache.parquet.thrift.projection.deprecated.DeprecatedFieldProjectionFilter</exclude>


https://github.com/apache/parquet-mr/blob/5ee513341158bc3962df80260c345b80076dbf3e/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/deprecated/DeprecatedFieldProjectionFilter.java#L33-L34

Fokko · 2023-10-18T16:05:26Z

pom.xml

+              Got this from:
+                org.apache.parquet.thrift.projection.FieldProjectionFilter[org.apache.parquet.thrift.projection.FieldProjectionFilter]:INTERFACE_REMOVED
+
+              But it didn't change, only the @Deprecated annotation was added


Maybe it is a bug, let's try to bump it: #1176

Nice, the bump fixed it :)

wgtmac · 2023-10-20T01:59:04Z

...in/java/org/apache/parquet/thrift/projection/deprecated/DeprecatedFieldProjectionFilter.java

- * This is used for parsing values assigned to ThriftReadSupport.THRIFT_COLUMN_FILTER_KEY
- */
-@Deprecated
-public class DeprecatedFieldProjectionFilter implements FieldProjectionFilter {


Why is this file removed?

These classes are marked as deprecated in 2015, and released in Parquet 1.8.0 :) Removing these was also suggested by @steveloughran in #979

wgtmac · 2023-10-20T02:01:10Z

parquet-thrift/src/main/java/org/apache/parquet/hadoop/thrift/ThriftReadSupport.java

-   * Deprecated. Use {@link #STRICT_THRIFT_COLUMN_FILTER_KEY}
-   * Accepts a ";" delimited list of globs in the syntax implemented by {@link DeprecatedFieldProjectionFilter}
-   */
-  @Deprecated


Why not keeping them as-is?

These are marked as deprecated in 2015. I think removing these is a first step before removing the whole class.

part of my attempts to move up to later hadoop versions hit real problems here. and along with thrift in general -i had to grab a copy of an x86 MBP for my mac m1, but even there homebrew had decided to upgrade it, so it was a backup in a corner I ended up with. At which point I discovered that homebrew isn't a real package manager as you can't roll back...

I'm also on a M1, and it takes quite a bit of effort to get Thrift compiled. I got a patch ready to update Thrift to the latest version. I'm not sure if we need to update parquet-format first, since the release is still on an older version. I'll give it a try in a moment.

that or stick the old version into the repo...

Fokko · 2023-10-20T09:27:44Z

PTAL @shangxinli @gszadovszky @SinghAsDev @rdblue @julienledem

gszadovszky · 2023-10-20T10:31:44Z

@Fokko,
If you want to deprecate the whole module I think, it is better to do something like we have done with parquet-tools and other before. See #851.
It might also worth a note on the dev list to raise awareness.

…te-thrift

SinghAsDev · 2023-10-20T16:31:41Z

Hi @Fokko thanks for bringing this up. We at Pinterest still use Parquet thrift quite extensively. How urgent is thrift 0.19.0 upgrade and how much time can we wait before we absolutely need to upgrade thrift? Also, since elephantbird is not maintained anymore, one option would be to just get rid of elephantbird dependency completely, even if it means moving some code in parquet-mr. Thoughts?

SinghAsDev · 2023-10-20T23:46:47Z

@tlazaro, @ttim, @piyushnarang, @isnotinvain in case you all still use parquet-thrift, this discussion maybe of interest to you as well.

tlazaro · 2023-10-21T01:30:46Z

Thanks so much for pinging, though I'm no longer at Twitter, I will try to find someone who might be interested in the conversation.

Fokko · 2023-11-08T08:41:27Z

@SinghAsDev and @tlazaro I think we can make it work with the latest version of Elephantbird. Let me close this for now.

Fokko added 2 commits October 18, 2023 15:43

PARQUET-2355: Deprecate parquet-thrift

16a1aa5

Remove deprecated parts

bc38a73

Fokko force-pushed the fd-deprecate-thrift branch from e60d633 to bc38a73 Compare October 18, 2023 14:43

Remove deprecated tests

460edb3

Fokko force-pushed the fd-deprecate-thrift branch from a5b9f1c to 460edb3 Compare October 18, 2023 15:11

Add japicmp exclusions

bde22f1

Fokko commented Oct 18, 2023

View reviewed changes

Fokko force-pushed the fd-deprecate-thrift branch 2 times, most recently from b67bf21 to 52472b3 Compare October 18, 2023 21:02

Update syntax

857b84c

Fokko force-pushed the fd-deprecate-thrift branch from 52472b3 to 857b84c Compare October 18, 2023 21:52

wgtmac reviewed Oct 20, 2023

View reviewed changes

Move the artifact

09c3d41

Fokko force-pushed the fd-deprecate-thrift branch from a3961d4 to 09c3d41 Compare October 20, 2023 12:05

Merge branch 'master' of github.com:apache/parquet-mr into fd-depreca…

830b381

…te-thrift

Fokko closed this Nov 8, 2023

Conversation

Fokko commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Jira

Tests

Commits

Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fokko commented Oct 20, 2023

Uh oh!

gszadovszky commented Oct 20, 2023

Uh oh!

SinghAsDev commented Oct 20, 2023

Uh oh!

SinghAsDev commented Oct 20, 2023

Uh oh!

tlazaro commented Oct 21, 2023

Uh oh!

Fokko commented Nov 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Fokko commented Oct 18, 2023 •

edited

Loading