You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2023/06/19 06:03:15 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #41654: [SPARK-44064][CORE][SQL] Add a new `apply` function to `NonFateSharingCache`
LuciferYang opened a new pull request, #41654:
URL: https://github.com/apache/spark/pull/41654
### What changes were proposed in this pull request?
This pr add a new `apply` function to `NonFateSharingCache` and change `CodeGenerator` to use the new `NonFateSharingCache#apply` to make `catalyst` module can test using maven.
### Why are the changes needed?
SPARK-43300 introduced `NonFateSharingCache` to `core` module and it only used by `CodeGenerator` which is in `catalyst` module.
There are two `apply` funcitons in `NonFateSharingCache` and the input parameter of `NonFateSharingCache#apply` is `com.google.common.cache.Cache/LoadingCache`.
The `catalyst` module may use shaded spark-core jar when we do maven testing and in the shaded spark-core jar, the Guava related classes will be relocated from `com.google.common` to `org.sparkproject.guava`, so the input parameter of `NonFateSharingCache#apply` will change to `org.sparkproject.guava.cache.Cache/LoadingCache`, but the catalyst module has not been shaded yet when do maven testing, so `CodeGenerator` will still use type `com.google.common.cache.Cache` to call the `NonFateSharingCache#apply` function, then this will result in a mismatch of input types when do maven testing and maven test will aborted as follows:
```
ProductAggSuite:
*** RUN ABORTED ***
java.lang.NoClassDefFoundError: Could not initialize class org.apache.spark.sql.catalyst.expressions.codegen.CodeGenerator$
at org.apache.spark.sql.catalyst.expressions.codegen.JavaCode$.variable(javaCode.scala:64)
at org.apache.spark.sql.catalyst.expressions.codegen.JavaCode$.isNullVariable(javaCode.scala:77)
at org.apache.spark.sql.catalyst.expressions.Expression.$anonfun$genCode$3(Expression.scala:200)
at scala.Option.getOrElse(Option.scala:189)
at org.apache.spark.sql.catalyst.expressions.Expression.genCode(Expression.scala:196)
at org.apache.spark.sql.catalyst.expressions.codegen.GenerateSafeProjection$.$anonfun$create$1(GenerateSafeProjection.scala:156)
at scala.collection.immutable.List.map(List.scala:293)
at org.apache.spark.sql.catalyst.expressions.codegen.GenerateSafeProjection$.create(GenerateSafeProjection.scala:153)
at org.apache.spark.sql.catalyst.expressions.codegen.GenerateSafeProjection$.create(GenerateSafeProjection.scala:39)
at org.apache.spark.sql.catalyst.expressions.codegen.CodeGenerator.generate(CodeGenerator.scala:1369)
```
So this pr add a new `apply` function to break non-core modules directly using Guava Cache related types as input parameter to invoke `NonFateSharingCache#apply` function for workaround, this way can avoid non-core modules Maven test failures caused by using shaded core module.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
- Pass GitHub Actions
- Manual test
```
build/mvn clean install -DskipTests -pl sql/catalyst -am
build/mvn test -pl sql/catalyst
```
Before
```
ProductAggSuite:
*** RUN ABORTED ***
java.lang.NoClassDefFoundError: Could not initialize class org.apache.spark.sql.catalyst.expressions.codegen.CodeGenerator$
at org.apache.spark.sql.catalyst.expressions.codegen.JavaCode$.variable(javaCode.scala:64)
at org.apache.spark.sql.catalyst.expressions.codegen.JavaCode$.isNullVariable(javaCode.scala:77)
at org.apache.spark.sql.catalyst.expressions.Expression.$anonfun$genCode$3(Expression.scala:200)
at scala.Option.getOrElse(Option.scala:189)
at org.apache.spark.sql.catalyst.expressions.Expression.genCode(Expression.scala:196)
at org.apache.spark.sql.catalyst.expressions.codegen.GenerateSafeProjection$.$anonfun$create$1(GenerateSafeProjection.scala:156)
at scala.collection.immutable.List.map(List.scala:293)
at org.apache.spark.sql.catalyst.expressions.codegen.GenerateSafeProjection$.create(GenerateSafeProjection.scala:153)
at org.apache.spark.sql.catalyst.expressions.codegen.GenerateSafeProjection$.create(GenerateSafeProjection.scala:39)
at org.apache.spark.sql.catalyst.expressions.codegen.CodeGenerator.generate(CodeGenerator.scala:1369)
```
After
```
Run completed in 4 minutes, 22 seconds.
Total number of tests run: 7088
Suites: completed 295, aborted 0
Tests: succeeded 7088, failed 0, canceled 0, ignored 5, pending 0
All tests passed.
```
--
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] LuciferYang commented on pull request #41654: [SPARK-44064][CORE][SQL] Add a new `apply` function to `NonFateSharingCache`
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41654:
URL: https://github.com/apache/spark/pull/41654#issuecomment-1598219946
Thanks @HyukjinKwon for your review, friendly ping @JoshRosen @liuzqt
--
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] LuciferYang commented on pull request #41654: [SPARK-44064][CORE][SQL] Add a new `apply` function to `NonFateSharingCache`
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41654:
URL: https://github.com/apache/spark/pull/41654#issuecomment-1600615010
+1, Agree with what @HyukjinKwon said
--
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] LuciferYang commented on pull request #41654: [SPARK-44064][CORE][SQL] Add a new `apply` function to `NonFateSharingCache`
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41654:
URL: https://github.com/apache/spark/pull/41654#issuecomment-1605830267
> Hi @LuciferYang , thanks for the fix! I'm fine with either option.
I rebase the code to make GA test this one again. @HyukjinKwon seems the author approves of this fix. I am planning to merge this one today, do you think it's ok?
--
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] LuciferYang commented on pull request #41654: [SPARK-44064][CORE][SQL] Add a new `apply` function to `NonFateSharingCache`
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41654:
URL: https://github.com/apache/spark/pull/41654#issuecomment-1606438173
Thanks @dongjoon-hyun @HyukjinKwon @liuzqt @ryan-johnson-databricks
--
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] ryan-johnson-databricks commented on pull request #41654: [SPARK-44064][CORE][SQL] Add a new `apply` function to `NonFateSharingCache`
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on PR #41654:
URL: https://github.com/apache/spark/pull/41654#issuecomment-1599468238
> friendly ping @HyukjinKwon @dongjoon-hyun , I want to use this one instead of #41622
Could you explain why you favor the more complex approach that changes prod code to address a test-only issue?
All else being equal, the other PR should merge because it's a lot simpler.
--
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] dongjoon-hyun closed pull request #41654: [SPARK-44064][CORE][SQL] Add a new `apply` function to `NonFateSharingCache`
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #41654: [SPARK-44064][CORE][SQL] Add a new `apply` function to `NonFateSharingCache`
URL: https://github.com/apache/spark/pull/41654
--
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] HyukjinKwon commented on pull request #41654: [SPARK-44064][CORE][SQL] Add a new `apply` function to `NonFateSharingCache`
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #41654:
URL: https://github.com/apache/spark/pull/41654#issuecomment-1599747544
I do't mind either this or https://github.com/apache/spark/pull/41622. I think the main reason is to keep `NonFateSharingCache.scala` in Spark core module instead of moving it to SQL module because this looks more for a general purpose. So TBH I am fine which one to merge. It's not an API in the end.
--
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] liuzqt commented on pull request #41654: [SPARK-44064][CORE][SQL] Add a new `apply` function to `NonFateSharingCache`
Posted by "liuzqt (via GitHub)" <gi...@apache.org>.
liuzqt commented on PR #41654:
URL: https://github.com/apache/spark/pull/41654#issuecomment-1601753830
Hi @LuciferYang , thanks for the fix! I'm fine with either option.
--
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] LuciferYang commented on pull request #41654: [SPARK-44064][CORE][SQL] Add a new `apply` function to `NonFateSharingCache`
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41654:
URL: https://github.com/apache/spark/pull/41654#issuecomment-1598086203
friendly ping @HyukjinKwon @dongjoon-hyun , I want to use this one instead of https://github.com/apache/spark/pull/41622
--
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] LuciferYang commented on pull request #41654: [SPARK-44064][CORE][SQL] Add a new `apply` function to `NonFateSharingCache`
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41654:
URL: https://github.com/apache/spark/pull/41654#issuecomment-1605829615
Thanks @liuzqt
--
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