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

ORC-1019: Remove redundant jackson dependencies #930

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

guiyanakuang
Copy link
Member

@guiyanakuang guiyanakuang commented Oct 5, 2021

What changes were proposed in this pull request?

This PR aims to remove redundant jackson dependencies.
Unfortunately, ORC-946 forgot to remove the bench dependency on jackson. In fact, the bench module does not directly depend on jackson, only spark indirectly depends on the specified version of jackson.

Why are the changes needed?

Minimising dependencies.

How was this patch tested?

Pass the CIs.

Closes #928

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Thank you for making a PR, @guiyanakuang . BTW, this looks like a revert of ORC-807 (from @belugabehr )

So, is this the current status, @guiyanakuang ?

In fact, the bench module does not directly depend on jackson, only spark indirectly depends on the specified version of jackson.

@guiyanakuang
Copy link
Member Author

guiyanakuang commented Oct 6, 2021

Thank you for making a PR, @guiyanakuang . BTW, this looks like a revert of ORC-807 (from @belugabehr )

So, is this the current status, @guiyanakuang ?

In fact, the bench module does not directly depend on jackson, only spark indirectly depends on the specified version of jackson.

Yes, because the code that depends on jackson has been rewritten with gson in ORC-946. I'm sure this is the current state.

Comment before update: which reminds me that we don't even need to declare Spark's dependency on jackson, there is no conflict.
I immediately made a new commit to remove the explicit dependency of spark on jackson.

update: I was wrong, actually spark and avro depend on different versions of jackson in orc-benchmarks-spark, explicit dependencies are better than letting maven choose based on the length of the dependency path, I think it's better to keep it.

@dongjoon-hyun
Copy link
Member

Ya, I agree with your updated opinion, @guiyanakuang . Then, shall we close this PR?

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Oct 6, 2021

Since you investigated this, please take over #928.
If you make a PR, I will merge yours.

The bot is only reducing the human-labor to check the new versions. So, the main contribution is the human review and investigation. Thank you for your review on #928 and investigation on this. I really appreciate your passion and contribution, @guiyanakuang .

@guiyanakuang
Copy link
Member Author

@dongjoon-hyun. Sorry, I didn't make my point clearly.
My updated view is to keep the orc-benchmarks-spark dependency unchanged, but we can remove the jackson dependency from the overall definition of the bench, as we don't explicitly use jakcson in the bench anymore, and the previous part in orc-benchmarks-core was already rewritten using gson rewrites the So my view is to use this pr instead of bot. This pr still retains the explicit specification of jackson by orc-benchmarks-spark. 😄

@dongjoon-hyun
Copy link
Member

Got it, @guiyanakuang !

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM.

@dongjoon-hyun dongjoon-hyun merged commit 93af6b0 into apache:main Oct 6, 2021
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Oct 6, 2021

Merged to main for Apache ORC 1.8.

BTW, one tip for you, @guiyanakuang .
When we override another PR, you can use Closes #928 at the bottom of PR description.
It will close that specific PR when this PR get merged.

For this PR, I revised the PR description and merged it.

@guiyanakuang
Copy link
Member Author

Merged to main for Apache ORC 1.8.

BTW, one tip for you, @guiyanakuang . When we override another PR, you can use Closes #928 at the bottom of PR description. It will close that specific PR when this PR get merged.

For this PR, I revised the PR description and merged it.

Got it, hahaha, thank you @dongjoon-hyun .

@dongjoon-hyun dongjoon-hyun added this to the 1.8.0 milestone Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants