-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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
[SPARK-46679][SQL] Fix for SparkUnsupportedOperationException Not found an encoder of the type T, when using Parameterized class #48304
base: master
Are you sure you want to change the base?
Conversation
…on: [ENCODER_NOT_FOUND] Not found an encoder of the type T
…me data class with different parameterized types
I have a small suggestion on simplifying the test case, but overall I think this is good. The included test case does cover behavior which worked in Spark 3.3, and regressed in spark 3.4 (the test case can't be directly ported, exactly as is, but I tried something similar to it in Spark 3.3 and confirmed that it worked there). |
lgtm |
test failures are unrelated, failed to download an artifact |
} | ||
|
||
test("SPARK-46679: resolve generics with multi-level inheritance same type names") { | ||
val encoder = JavaTypeInference.encoderFor(classOf[CompanyWrapperT]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent a bit of time playing around with the test bean classes, and I think the actual minimal example to reproduce the bug is
static class Foo<T> {
T t;
public void setT(T t) {
this.t = t;
}
public T getT() {
return t;
}
}
static class InnerWrapper<U> {
Foo<U> foo;
public void setFoo(Foo<U> foo) {
this.foo = foo;
}
public Foo<U> getFoo() {
return foo;
}
}
static class OuterWrapper extends InnerWrapper<String> {}
while I know this test case came from the user's report, I think it would actually be easier for another developer to understand the point of this test case with the more minimal example.
I do think there is value in having the test cases with both combos of names for the type parameters still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, sorry ignore this, I made a mistake posting my comments to github earlier. I meant to post this, but I think the simplification done already is fine
What changes were proposed in this pull request?
The change involves pushing the concrete class obtained from parameterized type variable and any introspected type variables in the concrete class's hierarchy, in the mappings so that encoder can be created.
To further illustrate
With this change, an instance of OuterWrapper has the String type pushed through to myFoo.myT, and the correct encoder is generated.
Why are the changes needed?
To fix the issue tracked by bug SPARK-46679
So that appropriate encoder can be created.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Added bug tests and additional tests
Was this patch authored or co-authored using generative AI tooling?
No.
But the original fix was created by @squito and I verified it using extra testing.