You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "gbloisi-openaire (via GitHub)" <gi...@apache.org> on 2023/08/03 21:51:41 UTC
[GitHub] [spark] gbloisi-openaire opened a new pull request, #42327: [SPARK-44634][SQL] Encoders.bean does no longer support nested beans with type arguments
gbloisi-openaire opened a new pull request, #42327:
URL: https://github.com/apache/spark/pull/42327
### What changes were proposed in this pull request?
This PR fixes a regression introduced in Spark 3.4.x where Encoders.bean is no longer able to process nested beans having type arguments. For example:
```
class A<T> {
T value;
// value getter and setter
}
class B {
A<String> stringHolder;
// stringHolder getter and setter
}
Encoders.bean(B.class); // throws "SparkUnsupportedOperationException: [ENCODER_NOT_FOUND]..."
```
### Why are the changes needed?
JavaTypeInference.encoderFor main match does not manage ParameterizedType and TypeVariable cases. I think this is a regression introduced after getting rid of usage of guava TypeToken.
In this PR I'm leveraging commons lang3 TypeUtils functionalities to solve ParameterizedType type arguments
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing tests have been extended to check correct encoding of a nested bean having type arguments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hvanhovell commented on pull request #42327: [SPARK-44634][SQL] Encoders.bean does no longer support nested beans with type arguments
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #42327:
URL: https://github.com/apache/spark/pull/42327#issuecomment-1666953531
@gbloisi-openaire can you open a port to 3.4?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gbloisi-openaire commented on a diff in pull request #42327: [SPARK-44634][SQL] Encoders.bean does no longer support nested beans with type arguments
Posted by "gbloisi-openaire (via GitHub)" <gi...@apache.org>.
gbloisi-openaire commented on code in PR #42327:
URL: https://github.com/apache/spark/pull/42327#discussion_r1284792734
##########
sql/catalyst/pom.xml:
##########
@@ -116,6 +116,10 @@
<artifactId>datasketches-java</artifactId>
<version>3.3.0</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-lang3</artifactId>
Review Comment:
Yes, the addition can be removed. I just committed the change. It was likely added in the wrong place because I was porting my changes from the 3.4 branch where I worked originally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hvanhovell commented on pull request #42327: [SPARK-44634][SQL] Encoders.bean does no longer support nested beans with type arguments
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #42327:
URL: https://github.com/apache/spark/pull/42327#issuecomment-1666953338
Merging to master/3.5.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hvanhovell closed pull request #42327: [SPARK-44634][SQL] Encoders.bean does no longer support nested beans with type arguments
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell closed pull request #42327: [SPARK-44634][SQL] Encoders.bean does no longer support nested beans with type arguments
URL: https://github.com/apache/spark/pull/42327
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hvanhovell commented on a diff in pull request #42327: [SPARK-44634][SQL] Encoders.bean does no longer support nested beans with type arguments
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #42327:
URL: https://github.com/apache/spark/pull/42327#discussion_r1284568815
##########
sql/catalyst/pom.xml:
##########
@@ -116,6 +116,10 @@
<artifactId>datasketches-java</artifactId>
<version>3.3.0</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-lang3</artifactId>
Review Comment:
Why do we need to add this to catalyst? The change is in sql/api...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hvanhovell commented on a diff in pull request #42327: [SPARK-44634][SQL] Encoders.bean does no longer support nested beans with type arguments
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #42327:
URL: https://github.com/apache/spark/pull/42327#discussion_r1284567838
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala:
##########
Review Comment:
Should we replace whatever I tried in `ImplementsGenericInterface` with the commons-lang utils?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gbloisi-openaire commented on a diff in pull request #42327: [SPARK-44634][SQL] Encoders.bean does no longer support nested beans with type arguments
Posted by "gbloisi-openaire (via GitHub)" <gi...@apache.org>.
gbloisi-openaire commented on code in PR #42327:
URL: https://github.com/apache/spark/pull/42327#discussion_r1284792298
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala:
##########
Review Comment:
Sure. Thanks for pointing that, I just committed the change. I didn't realize the change could be easily reused for lists and maps, reducing the code a lot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hvanhovell commented on pull request #42327: [SPARK-44634][SQL] Encoders.bean does no longer support nested beans with type arguments
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #42327:
URL: https://github.com/apache/spark/pull/42327#issuecomment-1665776180
@gbloisi-openaire thanks for doing this. I will take a look next week.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gbloisi-openaire commented on pull request #42327: [SPARK-44634][SQL] Encoders.bean does no longer support nested beans with type arguments
Posted by "gbloisi-openaire (via GitHub)" <gi...@apache.org>.
gbloisi-openaire commented on PR #42327:
URL: https://github.com/apache/spark/pull/42327#issuecomment-1667723220
Port of this to branch-3.4 is here [42379](https://github.com/apache/spark/pull/42379)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hvanhovell commented on a diff in pull request #42327: [SPARK-44634][SQL] Encoders.bean does no longer support nested beans with type arguments
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #42327:
URL: https://github.com/apache/spark/pull/42327#discussion_r1284564794
##########
sql/catalyst/pom.xml:
##########
@@ -116,6 +116,10 @@
<artifactId>datasketches-java</artifactId>
<version>3.3.0</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-lang3</artifactId>
Review Comment:
Isn't this already included transitively?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org