Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PARQUET-2344: Bump to Thrift 0.19.0 #1192

Merged
merged 5 commits into from
Jan 29, 2024
Merged

PARQUET-2344: Bump to Thrift 0.19.0 #1192

merged 5 commits into from
Jan 29, 2024

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Nov 13, 2023

Make sure you have checked all steps below.

This requires a new release of parquet-format, but it looks like @wgtmac is on top of that: apache/parquet-format#197 (comment)

Jira

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

@wgtmac
Copy link
Member

wgtmac commented Nov 14, 2023

Do you need any additional fix to parquet-format before the v2.10 release? @Fokko

@Fokko
Copy link
Contributor Author

Fokko commented Nov 14, 2023

@wgtmac No just the release, thanks for asking :)

@wgtmac
Copy link
Member

wgtmac commented Nov 15, 2023

@wgtmac No just the release, thanks for asking :)

Thanks for confirmation!

@wgtmac
Copy link
Member

wgtmac commented Nov 21, 2023

@Fokko Do you want to include parquet-format 2.10.0 adoption in this PR or create a separate one? IMO, there are several PRs waiting for 2.10.0 to rebase so it would be good to check that in first?

@Fokko Fokko force-pushed the fd-bump-thriftt branch 3 times, most recently from 9d29b90 to 264b859 Compare November 27, 2023 23:25
@Fokko
Copy link
Contributor Author

Fokko commented Nov 28, 2023

@wgtmac Thanks for splitting out the format upgrade. Always a good idea to make PRs smaller.

I finally fixed all the tests, and this looks good to go to me 👍

@Fokko Fokko requested a review from wgtmac December 1, 2023 17:58
wgtmac
wgtmac reviewed Dec 4, 2023
View reviewed changes
parquet-thrift/src/main/java/org/apache/parquet/thrift/ThriftSchemaConverter.java
List<EnumValue> values = new ArrayList<ThriftType.EnumValue>();
for (TEnum tEnum : enumValues) {
values.add(new EnumValue(tEnum.getValue(), tEnum.toString()));
if (field.isEnum()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why mixing UUID and ENUM in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there are still enums with field-id 16. It can be that they are using an old Java SDK. This was a reliable way do differentiate between them.

pom.xml
@@ -619,6 +622,9 @@
<configuration>
<failOnWarning>true</failOnWarning>
<ignoreNonCompile>true</ignoreNonCompile>
<ignoredDependencies>
<dependency>javax.annotation:javax.annotation-api:jar:1.3.2</dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to ignore this?

parquet-format-structures/pom.xml
@@ -156,6 +156,11 @@
<artifactId>libthrift</artifactId>
<version>${format.thrift.version}</version>
</dependency>
<dependency>
<groupId>javax.annotation</groupId>
<artifactId>javax.annotation-api</artifactId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do we need this?

parquet-thrift/src/main/java/org/apache/parquet/thrift/struct/ThriftTypeID.java Show resolved Hide resolved
@Fokko Fokko requested a review from wgtmac January 3, 2024 08:28
@wgtmac
Copy link
Member

wgtmac commented Jan 28, 2024

Do you want to merge this?

@Fokko
Copy link
Contributor Author

Fokko commented Jan 29, 2024

@wgtmac Yes, I'd love to. Thanks 🙌

@Fokko Fokko merged commit 15ad966 into apache:master Jan 29, 2024
9 checks passed
@Fokko Fokko deleted the fd-bump-thriftt branch January 29, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wgtmac wgtmac wgtmac approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Fokko @wgtmac