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.

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

Upgrade parquet dependencies to 1.14.1 #1107

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chandrashekar-s
Copy link
Collaborator

@chandrashekar-s chandrashekar-s commented Jul 1, 2024

Description of what I changed

This PR upgrades parquet-avro and its transitive dependencies to the same version, which fixes the error introduced by this PR.

E2E test

Relied on automated e2e tests.

TESTED:

NA

Checklist: I completed these to help reviewers :)

  • I have read and will follow the review process.

  • I am familiar with Google Style Guides for the language I have coded in.

    No? Please take some time and review Java and Python style guides.

  • My IDE is configured to follow the Google code styles.

    No? Unsure? -> configure your IDE.

  • I have added tests to cover my changes. (If you refactored existing code that was well tested you do not have to add tests)

  • I ran mvn clean package right before creating this pull request and added all formatting changes to my commit.

  • All new and existing tests passed.

  • My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.00%. Comparing base (2595915) to head (1b7e691).

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1107      +/-   ##
============================================
- Coverage     52.02%   52.00%   -0.02%     
+ Complexity      655      654       -1     
============================================
  Files            89       89              
  Lines          5388     5388              
  Branches        708      708              
============================================
- Hits           2803     2802       -1     
  Misses         2323     2323              
- Partials        262      263       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

</dependency>

<dependency>
<groupId>org.apache.parquet</groupId>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These dependencies are transitive dependencies of beam-sdks-java-io-parquet and were resolving to a different version than the parquet-avro. So, locking all the dependencies to the same version at the root package.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is okay but one problem that I see is that Beam's code may start throwing exceptions if we use a difference Parquet lib version. Although this issue is not unique here, it might be worth trying to fix avoid it. One option is to remove these new deps and only change parquet.version when we change Beam's version and keep them consistent; WDTY?

@chandrashekar-s
Copy link
Collaborator Author

chandrashekar-s commented Jul 1, 2024

@bashir2 This PR resolves the upgrade issue caused by this PR. Please review.

Copy link
Collaborator

@bashir2 bashir2 left a comment

Choose a reason for hiding this comment

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

Thanks @chandrashekar-s for looking into version upgrade issues; please see my comment below:

</dependency>

<dependency>
<groupId>org.apache.parquet</groupId>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is okay but one problem that I see is that Beam's code may start throwing exceptions if we use a difference Parquet lib version. Although this issue is not unique here, it might be worth trying to fix avoid it. One option is to remove these new deps and only change parquet.version when we change Beam's version and keep them consistent; WDTY?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants