Skip to content

Conversation

@pan3793
Copy link
Member

@pan3793 pan3793 commented Jul 31, 2025

Rationale for this change

Fix #3253

What changes are included in this PR?

As the title.

Are these changes tested?

Before
image

After
Xnip2025-07-31_17-08-35

Are there any user-facing changes?

No.

Closes #3253

@pan3793
Copy link
Member Author

pan3793 commented Jul 31, 2025

cc @LDVSOFT who reports this issue

@pan3793
Copy link
Member Author

pan3793 commented Aug 1, 2025

cc @wgtmac

@wgtmac wgtmac requested a review from gszadovszky August 2, 2025 16:10
@wgtmac
Copy link
Member

wgtmac commented Aug 2, 2025

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

@gszadovszky
Copy link
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
Copy link
Member Author

pan3793 commented Aug 4, 2025

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.

@gszadovszky
Copy link
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
Copy link
Member Author

pan3793 commented Aug 4, 2025

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.

image

Copy link
Contributor

@gszadovszky gszadovszky 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, @pan3793 for the clarification. I'm OK getting this in.

@wgtmac 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
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