FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-3253: Apply ServicesResourceTransformer to parquet-jackson by pan3793 · Pull Request #3260 · apache/parquet-java · GitHub

GH-3253: Apply ServicesResourceTransformer to parquet-jackson - #3260

Merged
wgtmac merged 1 commit into
apache:masterfrom
pan3793:GH-3253
Aug 21, 2025
Merged

GH-3253: Apply ServicesResourceTransformer to parquet-jackson#3260
wgtmac merged 1 commit into
apache:masterfrom
pan3793:GH-3253

Conversation

pan3793 commented Jul 31, 2025

Copy link
Copy Markdown
Member

Rationale for this change

Fix #3253

What changes are included in this PR?

As the title.

Are these changes tested?

Before

After

Are there any user-facing changes?

No.

Closes #3253

pan3793 commented Jul 31, 2025

Copy link
Copy Markdown
Member Author

cc @LDVSOFT who reports this issue

pan3793 commented Aug 1, 2025

Copy link
Copy Markdown
Member Author

cc @wgtmac

wgtmac requested a review from gszadovszky August 2, 2025 16:10

wgtmac commented Aug 2, 2025

Copy link
Copy Markdown
Member

I'm not familiar with this. Could you help review this? @gszadovszky

Copy link
Copy Markdown
Contributor

@pan3793, could you elaborate a bit more on what is happening here? The jackson based services are not available currently because of the shading, right? Which means, that parquet-java does not use them. We do not want to make them available. We are shading jackson, so systems depending on parquet-java would not use it directly.

pan3793 commented Aug 4, 2025

Copy link
Copy Markdown
Member Author

parquet-java does not use them

@gszadovszky Probably right, because I haven't seen user reports issues related to that. But this can not be proven by UT, because currently, Maven UT always runs against vanilla Jackson libs, class relocation only happens on packaging.

Anyway, we should either correctly transform those service files or purge them.

Generally, applying ServicesResourceTransformer is preferred to keep the functionality completeness of the shaded libs, for example, we generally keep all classes of the shaded lib instead of keeping only the used classes.

Copy link
Copy Markdown
Contributor

@pan3793, can the shaded services interfere with a potential official Jackson library on the classpath? If not, I'm good with this. If yes, we should rather purge the services that we are not using.

pan3793 commented Aug 4, 2025

Copy link
Copy Markdown
Member Author

can the shaded services interfere with a potential official Jackson library on the classpath?

No, as long as ServicesResourceTransformer is applied, it relocates both the service interface and implementation class name.

gszadovszky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you, @pan3793 for the clarification. I'm OK getting this in.

wgtmac merged commit 7f36131 into apache:master Aug 21, 2025
7 checks passed
rahulketch pushed a commit to rahulketch/parquet-java that referenced this pull request Sep 9, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

jackson-parquet doesn't shade ServiceLoader discovery files

3 participants


Back | FazBrowse Home | New Git URL