Skip to content

Conversation

szehon-ho
Copy link
Member

@szehon-ho szehon-ho commented Sep 25, 2025

This pr is a rebase/continue of https://github.com/apache/spark/pull/48304/files, which was unexpectedly closed even though the issue is still there.

What changes were proposed in this pull request?

That pr fixes a bug in JavaTypeInference.encoderFor. In the parameterizableType case, it overrides the existing map of parameterizable type -> real type. So any nested parameterizable type will fail.

Why are the changes needed?

Fix nested parameterization cases for Java Encoder

Does this PR introduce any user-facing change?

No

How was this patch tested?

The original pr adds unit test to JavaTypeInferenceSuite and JavaDatasetSuite. The test from the original pr are simplified a little to make it obvious which one is wrapping.

Was this patch authored or co-authored using generative AI tooling?

No

…nd an encoder of the type T, when using Parameterized class
@github-actions github-actions bot added the SQL label Sep 25, 2025
@dongjoon-hyun
Copy link
Member

Could you re-trigger the failed 15 GitHub Action jobs? It seems to fail due to the Today's Docker Hub outage.


case pt: ParameterizedType =>
encoderFor(pt.getRawType, seenTypeSet, JavaTypeUtils.getTypeArguments(pt).asScala.toMap)
val newTvs = JavaTypeUtils.getTypeArguments(pt).asScala.toMap
Copy link
Member Author

@szehon-ho szehon-ho Sep 25, 2025

Choose a reason for hiding this comment

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

For reference, a walkthrough:

StringFooWrapper extends FooWrapper<String>
FooWrapper<U> contains Foo<U>
Foo<T>

On the first call for StringFooWrapper, typeVariable map contains (U => String)
On the next recursive call for FooWrapper, this gets overriden by (T => U)

The correct behavior is to keep both (U => String) and (T => U) in the final Encoder's typeVariable map.

@szehon-ho
Copy link
Member Author

@dongjoon-hyun thanks for the review! FYI i also did a minor cleanup of the original patch's map aggregation logic (should be the same behavior). Also previously test failure didnt look related, but re-triggering. also cc @cloud-fan fyi

@cloud-fan
Copy link
Contributor

The kafka test failure is unrelated, thanks, merging to master!

@cloud-fan cloud-fan closed this in 65b9da5 Sep 26, 2025
@szehon-ho
Copy link
Member Author

thank you @dongjoon-hyun @cloud-fan !

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

Successfully merging this pull request may close these issues.

3 participants